ping: return a stream of results #626
Conversation
Otherwise, we can't return errors. This is a breaking change but unlikely to have a large impact on anyone but go-ipfs. Part of ipfs/kubo#6298
ghost
assigned 
|
Going to leave this open for a day to collect feedback. |
| func (ps *PingService) Ping(ctx context.Context, p peer.ID) (<-chan time.Duration, error) { | ||
| // Result is a result of a ping attempt, either an RTT or an error. | ||
| type Result struct { | ||
| RTT time.Duration |
There was a problem hiding this comment.
We might want to return a time.Time representing the time the reading took place. The problem as I see it is that we perform pings eagerly, then wait reader to consume it off the channel. Time could've passed in between, and there's no deterministic way of telling the delta.
Alternatively we could wait for a reader to appear and send a thunk over the channel that will compute the ping, but that may be more complicated.
|
|
||
| for ctx.Err() == nil { | ||
| var res Result | ||
| res.RTT, res.Error = ping(s) |
There was a problem hiding this comment.
ping is untouched, but we should really set a read and write deadlines there. We could configure the ping service with a grace period, e.g. default 5 seconds, after which the ping will fail.
For inspiration, BSD ping uses 10 seconds:
There was a problem hiding this comment.
I agree. I'll handle that in a followup patch.
There was a problem hiding this comment.
Actually, I'm not sure how we should handle that. I'm starting to think we should call NewStream as needed, every time ping fails.
There was a problem hiding this comment.
Yeah, I would reuse the stream unless an error occurs, in which case I’d try to regenerate it, resetting the old one in a goroutine, while we call NewStream synchronously.
|
If writing to a stream fails, there’s something wrong with the underlying connection. In that case, I don’t think we want to open more streams. Returning the error seems to be the better solution. |
Otherwise, we can't return errors. This is a breaking change but unlikely to have a large impact on anyone but go-ipfs.
This also ensures canceling ping actually aborts the ping without waiting.
Part of ipfs/kubo#6298.