Update go-libp2p to latest #578
Conversation
gammazero
commented
Sep 5, 2024
gammazero
commented
- Update to go-libp2p v0.36.3
- Remove unnecessary timer channel read
- Update to go-libp2p v0.36.3 - Remove unnecessary timer channel read
discovery.go
Outdated
| if !t.Stop() { | ||
| <-t.C | ||
| } | ||
| t.Stop() |
There was a problem hiding this comment.
Out of curiosity, I would like to know why this introduces a bug. It seems to me that the purpose of this code is to create a stopped timer. The line immediately above the call to stop creates a timer with a timeout of 1 hour, so it is not conceivable that the timer's channel should ever need to be cleared.
Further, even if it were possible for more than an hour to elapse between these two consecutive lines of code, reading from the timer's channel would be a bug in go 1.23+, since trying to read from a timer's channel following a call to Stop will block forever.
There was a problem hiding this comment.
read more carefully, if Stop returns false you got to drain it.
There was a problem hiding this comment.
I did try to read carefully, Here it says this:
For a chan-based timer created with NewTimer(d), as of Go 1.23, any receive from t.C after Stop has returned is guaranteed to block rather than receive a stale time value from before the Stop; if the program has not received from t.C already and the timer is running, Stop is guaranteed to return true. Before Go 1.23, the only safe way to use Stop was insert an extra <-t.C if Stop returned false to drain a potential stale value. See the NewTimer documentation for more details.
Sorry if I am missing something.
FYI: Here is a way to handle the situation if multiple versions of go need to Stop and reuse timers:
https://github.com/quic-go/quic-go/pull/4659/files
if Stop returns false you got to drain it
That one hour timeout does a good job of ensuring that the immediately following call to Stop will never return false, so no need to drain even with go < 1.23.
There was a problem hiding this comment.
so they changed semantics for go 1.23 so as to break existing code that was following their recommemdations? Gross.
Ok fine, but still remove it from this pr and open a new one separately to have visibility and set the minimum version to 1.23; i dont like the 1hr workaround.
There was a problem hiding this comment.
I am trying to get a small repro to see if it's actually a golang bug. Ideally timer.Stop shouldn't return false if we haven't read anything.
The state of the draft PR https://github.com/quic-go/quic-go/pull/4659/files is correct though. You can rely on the capacity of the channel to handle this case.
There was a problem hiding this comment.
it can (at least before 1.23) return false if the timer has fired.
At any rate, lets make a separate pr for this and limit this one to the go-libp2p upgrade.
There was a problem hiding this comment.
it can (at least before 1.23) return false if the timer has fired.
Yes and it does.
From go1.23, it shouldn't.
