Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update go-libp2p to latest #578

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Update go-libp2p to latest #578

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
095b1bc
Update go-libp2p to latest
gammazero Sep 5, 2024
8b8c38c
Remove timer change from this PR
gammazero Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,7 @@ func (d *discover) Bootstrap(ctx context.Context, topic string, ready RouterRead
}

t := time.NewTimer(time.Hour)
if !t.Stop() {
<-t.C
}
t.Stop()
Copy link
Collaborator

Choose a reason for hiding this comment

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

please dont change this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

read more carefully, if Stop returns false you got to drain it.

Copy link
Contributor Author

@gammazero gammazero Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

@sukunrt sukunrt Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@vyzo vyzo Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

it can (at least before 1.23) return false if the timer has fired.

Yes and it does.

From go1.23, it shouldn't.

defer t.Stop()

for {
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ require (
github.com/gogo/protobuf v1.3.2
github.com/ipfs/go-log/v2 v2.5.1
github.com/libp2p/go-buffer-pool v0.1.0
github.com/libp2p/go-libp2p v0.36.2
github.com/libp2p/go-libp2p v0.36.3
github.com/libp2p/go-libp2p-testing v0.12.0
github.com/libp2p/go-msgio v0.3.0
github.com/multiformats/go-multiaddr v0.13.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ github.com/libp2p/go-buffer-pool v0.1.0 h1:oK4mSFcQz7cTQIfqbe4MIj9gLW+mnanjyFtc6
github.com/libp2p/go-buffer-pool v0.1.0/go.mod h1:N+vh8gMqimBzdKkSMVuydVDq+UV5QTWy5HSiZacSbPg=
github.com/libp2p/go-flow-metrics v0.1.0 h1:0iPhMI8PskQwzh57jB9WxIuIOQ0r+15PChFGkx3Q3WM=
github.com/libp2p/go-flow-metrics v0.1.0/go.mod h1:4Xi8MX8wj5aWNDAZttg6UPmc0ZrnFNsMtpsYUClFtro=
github.com/libp2p/go-libp2p v0.36.2 h1:BbqRkDaGC3/5xfaJakLV/BrpjlAuYqSB0lRvtzL3B/U=
github.com/libp2p/go-libp2p v0.36.2/go.mod h1:XO3joasRE4Eup8yCTTP/+kX+g92mOgRaadk46LmPhHY=
github.com/libp2p/go-libp2p v0.36.3 h1:NHz30+G7D8Y8YmznrVZZla0ofVANrvBl2c+oARfMeDQ=
github.com/libp2p/go-libp2p v0.36.3/go.mod h1:4Y5vFyCUiJuluEPmpnKYf6WFx5ViKPUYs/ixe9ANFZ8=
github.com/libp2p/go-libp2p-asn-util v0.4.1 h1:xqL7++IKD9TBFMgnLPZR6/6iYhawHKHl950SO9L6n94=
github.com/libp2p/go-libp2p-asn-util v0.4.1/go.mod h1:d/NI6XZ9qxw67b4e+NgpQexCIiFYJjErASrYW4PFDN8=
github.com/libp2p/go-libp2p-testing v0.12.0 h1:EPvBb4kKMWO29qP4mZGyhVzUyR25dvfUIK5WDu6iPUA=
Expand Down
Loading