swarm: track dial cancellation reason #2532
Conversation
sukunrt
force-pushed
the
sukun/swarm-metrics-close
branch
from
3665bd0 to
055cec0
Compare
p2p/net/swarm/swarm_metrics.go
Outdated
| if errors.Is(cause, errParentContextCanceled) { | ||
| // parent was canceled | ||
| e = "canceled" | ||
| } else if errors.Is(cause, context.Canceled) { |
There was a problem hiding this comment.
This looks brittle. Should we have a separate cause for this?
There was a problem hiding this comment.
Makes sense. That would be cleaner.
There was a problem hiding this comment.
I removed errParentContextCanceled and added errConcurrentDialSuccessful.
Now if the cause is context.Canceled it necessarily means the application cancelled the context and if the cause is errConcurrentDialSuccessful it means a concurrent dial succeeded.
| e = "canceled: dial successful" | ||
| } else { | ||
| // something else | ||
| e = "canceled: other" |
There was a problem hiding this comment.
Do we expect this to happen at all?
There was a problem hiding this comment.
Don't think so. Didnt happen on my kubo node. I ran it for about an hour
p2p/net/swarm/dial_sync.go
Outdated
| delete(ds.dials, p) | ||
| } | ||
| }() | ||
| return ad.dial(ctx) | ||
| conn, err := ad.dial(ctx) // updated err is used in defered func |
There was a problem hiding this comment.
Are you sure this works? Isn't := creating a new err here?
Why do we have this defer anyway, there's only a single return from this function.
There was a problem hiding this comment.
err is declared in the beginning of the method so this will assign to it.
Anyway, I will remove the defer. That is confusing.
sukunrt
force-pushed
the
sukun/swarm-metrics-close
branch
from
bb7cc59 to
69b01a6
Compare
| ad.refCnt-- | ||
| if ad.refCnt == 0 { | ||
| if err == nil { | ||
| ad.cancelCause(errConcurrentDialSuccessful) |
There was a problem hiding this comment.
Why? What would happen if we're just dialing one address, and that dial is successful?
There was a problem hiding this comment.
That dial would have succeeded so the context cancellation has no effect on that dial. That method would have returned.
There are two options:
- We set call
cancelCause(nil)and live with the brittle check thaterrors.Is(Cause(err), context.Canceled)signals concurrent successful dial. Here we override actual cancelations of parent context with errParentCanceled. This is the strategy in 055cec0 commit. - Is the latest commit. We explicitly signal
errConcurrentDialSuccessfulwhen err is nil. In case the dial succeeds, this is not a problem since we will not callFailedDialfor tracking metrics in that case.
closes: #2321