-
Notifications
You must be signed in to change notification settings - Fork 54
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.
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
chore: update to [email protected] #1428
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bassosimone
changed the title
chore: update dependencies
chore: update dependencies & other minor chores
Dec 13, 2023
24 tasks
This reverts commit da73fa7.
bassosimone
added a commit
that referenced
this pull request
Dec 13, 2023
Forked off the more complex #1428, which was failing. - run `./script/updateminipipeline.bash` - run `gofmt -s` - run `go generate ./...` - update user agent See ooni/probe#2556
bassosimone
force-pushed
the
issue/2556
branch
from
December 13, 2023 00:47
03401d8
to
186bb57
Compare
bassosimone
added a commit
that referenced
this pull request
Dec 13, 2023
Extracted from #1428 excluding packages causing tests to fail. Part of ooni/probe#2556.
bassosimone
changed the title
chore: update dependencies & other minor chores
chore: update to [email protected]
Dec 13, 2023
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
Forked off the more complex ooni#1428, which was failing. - run `./script/updateminipipeline.bash` - run `gofmt -s` - run `go generate ./...` - update user agent See ooni/probe#2556
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
Extracted from ooni#1428 excluding packages causing tests to fail. Part of ooni/probe#2556.
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
Upgrade to [email protected] and adapt closing policy after [changes in [email protected]](https://github.com/quic-go/quic-go/releases/tag/v0.40.0) and quic-go/quic-go#4072 in particular. We're creating an UDP conn and [passing it to server.Serve](https://github.com/ooni/probe-cli/pull/1428/files#diff-efda3daa51e9aed0b3444a327e64b7e5c412938a1fe894a3c850d533179c2425R105), which [calls serveConn](https://github.com/quic-go/quic-go/blob/v0.40.1/http3/server.go#L242), which [calls quicListen](https://github.com/quic-go/quic-go/blob/v0.40.1/http3/server.go#L316) and then [ServeListener](https://github.com/quic-go/quic-go/blob/v0.40.1/http3/server.go#L321). In turn, ServeListener [is interrupted by ErrServerClosed](https://github.com/quic-go/quic-go/blob/v0.40.1/http3/server.go#L268), which seems to be generated by [server.Close calling Close for each listener](https://github.com/quic-go/quic-go/blob/v0.40.1/http3/server.go#L657). The following is what happened before updating the shutdown protocol: ``` goroutine 247 [sync.Mutex.Lock]: [...] github.com/quic-go/quic-go.(*Transport).closeServer(0x1400054a000?) /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:298 +0x90 github.com/quic-go/quic-go.(*baseServer).close.func1() /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/server.go:344 +0x84 [...] github.com/quic-go/quic-go.(*baseServer).close(0x14000cc1c10?, {0x102faa7a0?, 0x140002557b0?}, 0xec?) /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/server.go:338 +0x64 github.com/quic-go/quic-go.(*baseServer).Close(...) /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/server.go:333 github.com/quic-go/quic-go.(*EarlyListener).Close(0x14000120700?) /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/server.go:165 +0x34 github.com/quic-go/quic-go/http3.(*Server).Close(0x140007344d0) /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/http3/server.go:657 +0xe8 github.com/ooni/probe-cli/v3/internal/netemx.(*http3Server).Close(0x140001345a0) /Users/sbs/src/github.com/ooni/probe-cli/internal/netemx/http3.go:66 +0xe4 github.com/ooni/probe-cli/v3/internal/netemx.(*QAEnv).Close.func1() /Users/sbs/src/github.com/ooni/probe-cli/internal/netemx/qaenv.go:291 +0x4c [...] github.com/ooni/probe-cli/v3/internal/netemx.(*QAEnv).Close(0x140001e5570?) /Users/sbs/src/github.com/ooni/probe-cli/internal/netemx/qaenv.go:288 +0x48 goroutine 136 [sync.Mutex.Lock]: [...] github.com/quic-go/quic-go.(*baseServer).close(0x14000127200?, {0x102faa7a0?, 0x140001920f0?}, 0xd8?) /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/server.go:338 +0x64 github.com/quic-go/quic-go.(*Transport).close(0x1400054a000, {0x102faa7a0, 0x140001920f0}) /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:325 +0x110 github.com/quic-go/quic-go.(*Transport).listen(0x1400054a000, {0x102fbd3a8, 0x14000526108}) /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:358 +0x364 created by github.com/quic-go/quic-go.(*Transport).init.func1 /Users/sbs/go/pkg/mod/github.com/quic-go/[email protected]/transport.go:242 +0x45c ``` Looking at the above traces, both end up at server.go:338 locking the same sync.Once. It seems the structures are such that attempting to close both the server and the listener leads to self locking in v0.40.{0,1}. The original expectation was that the server closed the connection used for listening anyway, and ooni/probe#2527 documented how that was not the case. It seems that now this is the case, so we can comment out the original ooni/probe#2527 fix without any test hangs. Also, if the original bug was indeed that the server did not own the listener, and considering that now it seems the server owns the listener, it makes sense that the fix for v0.40.1 is to revert the ooni/probe#2527 fix. See ooni/probe#2556
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upgrade to [email protected] and adapt closing policy after changes in [email protected] and quic-go/quic-go#4072 in particular. We're creating an UDP conn and passing it to server.Serve, which calls serveConn, which calls quicListen and then ServeListener. In turn, ServeListener is interrupted by ErrServerClosed, which seems to be generated by server.Close calling Close for each listener. The following is what happened before updating the shutdown protocol:
Looking at the above traces, both end up at server.go:338 locking the same sync.Once. It seems the structures are such that attempting to close both the server and the listener leads to self locking in v0.40.{0,1}. The original expectation was that the server closed the connection used for listening anyway, and ooni/probe#2527 documented how that was not the case. It seems that now this is the case, so we can comment out the original ooni/probe#2527 fix without any test hangs. Also, if the original bug was indeed that the server did not own the listener, and considering that now it seems the server owns the listener, it makes sense that the fix for v0.40.1 is to revert the ooni/probe#2527 fix.
See ooni/probe#2556