Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

dial: return a nice custom dial error #121

Merged
merged 1 commit into from
Apr 25, 2019
Merged

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Apr 24, 2019

  • Limits the number of dial errors we keep
  • Exposes the dial errors
  • Avoids string formatting unless we actually need to.

Helps address #119
Alternative to: #120

@ghost ghost assigned Stebalien Apr 24, 2019
@ghost ghost added the status/in-progress In progress label Apr 24, 2019
@Stebalien Stebalien requested review from raulk and vyzo April 24, 2019 22:28
* Limits the number of dial errors we keep
* Exposes the dial errors
* Avoids string formatting unless we actually need to.

Helps address #119
Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick question.


// TransportError is the error returned when dialing a specific address.
type TransportError struct {
Address ma.Multiaddr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to save the string representation of the multiaddr to avoid dereferencing and the interface cost? We're likely going to String() the maddr for logging anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. The cost of actually formatting the multiaddr will be higher and, hopefully, we'll only do that once anyways.

Really, we should avoid formatting any of this unless debug logging is enabled but that's going to involve ripping out or fixing our current logging framework.

connC, dialErr := s.dialAddrs(ctx, p, goodAddrsChan)
if dialErr != nil {
logdial["error"] = dialErr.Cause.Error()
if dialErr.Cause == context.Canceled {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a bug introduced by the multierror patch.

Copy link
Contributor

@vyzo vyzo Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@Stebalien Stebalien requested a review from raulk April 25, 2019 06:28
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fancy!

connC, dialErr := s.dialAddrs(ctx, p, goodAddrsChan)
if dialErr != nil {
logdial["error"] = dialErr.Cause.Error()
if dialErr.Cause == context.Canceled {
Copy link
Contributor

@vyzo vyzo Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@Stebalien Stebalien merged commit 0fc0136 into master Apr 25, 2019
@Stebalien Stebalien deleted the feat/improve-errors branch April 25, 2019 16:56
@ghost ghost removed the status/in-progress In progress label Apr 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants