Skip to content

Commit

Permalink
chore: update to [email protected] (#1428)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bassosimone
bassosimone authored Dec 13, 2023
1 parent 0c09e58 commit 29a5384
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 24 deletions.
4 changes: 2 additions & 2 deletions go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ require (
github.com/pborman/getopt/v2 v2.1.0
github.com/pion/stun v0.6.1
github.com/pkg/errors v0.9.1
github.com/quic-go/quic-go v0.39.0
github.com/quic-go/quic-go v0.40.1
github.com/rogpeppe/go-internal v1.11.0
github.com/rubenv/sql-migrate v1.5.2
github.com/schollz/progressbar/v3 v3.14.1
Expand Down Expand Up @@ -68,7 +68,7 @@ require (
github.com/pion/transport/v2 v2.2.4 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/quic-go/qpack v0.4.0 // indirect
github.com/quic-go/qtls-go1-20 v0.3.4 // indirect
github.com/quic-go/qtls-go1-20 v0.4.1 // indirect
github.com/refraction-networking/conjure v0.7.10-0.20231110193225-e4749a9dedc9 // indirect
github.com/refraction-networking/ed25519 v0.1.2 // indirect
github.com/refraction-networking/obfs4 v0.1.2 // indirect
Expand Down
8 changes: 4 additions & 4 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -415,10 +415,10 @@ github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k
github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo=
github.com/quic-go/qpack v0.4.0 h1:Cr9BXA1sQS2SmDUWjSofMPNKmvF6IiIfDRmgU0w1ZCo=
github.com/quic-go/qpack v0.4.0/go.mod h1:UZVnYIfi5GRk+zI9UMaCPsmZ2xKJP7XBUvVyT1Knj9A=
github.com/quic-go/qtls-go1-20 v0.3.4 h1:MfFAPULvst4yoMgY9QmtpYmfij/em7O8UUi+bNVm7Cg=
github.com/quic-go/qtls-go1-20 v0.3.4/go.mod h1:X9Nh97ZL80Z+bX/gUXMbipO6OxdiDi58b/fMC9mAL+k=
github.com/quic-go/quic-go v0.39.0 h1:AgP40iThFMY0bj8jGxROhw3S0FMGa8ryqsmi9tBH3So=
github.com/quic-go/quic-go v0.39.0/go.mod h1:T09QsDQWjLiQ74ZmacDfqZmhY/NLnw5BC40MANNNZ1Q=
github.com/quic-go/qtls-go1-20 v0.4.1 h1:D33340mCNDAIKBqXuAvexTNMUByrYmFYVfKfDN5nfFs=
github.com/quic-go/qtls-go1-20 v0.4.1/go.mod h1:X9Nh97ZL80Z+bX/gUXMbipO6OxdiDi58b/fMC9mAL+k=
github.com/quic-go/quic-go v0.40.1 h1:X3AGzUNFs0jVuO3esAGnTfvdgvL4fq655WaOi1snv1Q=
github.com/quic-go/quic-go v0.40.1/go.mod h1:PeN7kuVJ4xZbxSv/4OX6S1USOX8MJvydwpTx31vx60c=
github.com/refraction-networking/conjure v0.7.10-0.20231110193225-e4749a9dedc9 h1:46+z0lVJL5ynP09dtThex4GiowaANoBk7DsNF0+a+7Q=
github.com/refraction-networking/conjure v0.7.10-0.20231110193225-e4749a9dedc9/go.mod h1:O5u/Mpg5b3whLF8L701pTMQW23SviS+rDKdWbY/BM0Q=
github.com/refraction-networking/ed25519 v0.1.2 h1:08kJZUkAlY7a7cZGosl1teGytV+QEoNxPO7NnRvAB+g=
Expand Down
16 changes: 8 additions & 8 deletions internal/mocks/quic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ type QUICEarlyConnection struct {
MockConnectionState func() quic.ConnectionState
MockHandshakeComplete func() <-chan struct{}
MockNextConnection func() quic.Connection
MockSendMessage func(b []byte) error
MockReceiveMessage func(ctx context.Context) ([]byte, error)
MockSendDatagram func(b []byte) error
MockReceiveDatagram func(ctx context.Context) ([]byte, error)
}

var _ quic.EarlyConnection = &QUICEarlyConnection{}
Expand Down Expand Up @@ -121,14 +121,14 @@ func (s *QUICEarlyConnection) NextConnection() quic.Connection {
return s.MockNextConnection()
}

// SendMessage calls MockSendMessage.
func (s *QUICEarlyConnection) SendMessage(b []byte) error {
return s.MockSendMessage(b)
// SendDatagram calls MockSendDatagram.
func (s *QUICEarlyConnection) SendDatagram(b []byte) error {
return s.MockSendDatagram(b)
}

// ReceiveMessage calls MockReceiveMessage.
func (s *QUICEarlyConnection) ReceiveMessage(ctx context.Context) ([]byte, error) {
return s.MockReceiveMessage(ctx)
// ReceiveDatagram calls MockReceiveDatagram.
func (s *QUICEarlyConnection) ReceiveDatagram(ctx context.Context) ([]byte, error) {
return s.MockReceiveDatagram(ctx)
}

// UDPLikeConn is an UDP conn used by QUIC.
Expand Down
12 changes: 6 additions & 6 deletions internal/mocks/quic_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,29 +239,29 @@ func TestQUICEarlyConnection(t *testing.T) {
}
})

t.Run("SendMessage", func(t *testing.T) {
t.Run("SendDatagram", func(t *testing.T) {
expected := errors.New("mocked error")
qconn := &QUICEarlyConnection{
MockSendMessage: func(b []byte) error {
MockSendDatagram: func(b []byte) error {
return expected
},
}
b := make([]byte, 17)
err := qconn.SendMessage(b)
err := qconn.SendDatagram(b)
if !errors.Is(err, expected) {
t.Fatal("not the error we expected", err)
}
})

t.Run("ReceiveMessage", func(t *testing.T) {
t.Run("ReceiveDatagram", func(t *testing.T) {
expected := errors.New("mocked error")
ctx := context.Background()
qconn := &QUICEarlyConnection{
MockReceiveMessage: func(ctx context.Context) ([]byte, error) {
MockReceiveDatagram: func(ctx context.Context) ([]byte, error) {
return nil, expected
},
}
b, err := qconn.ReceiveMessage(ctx)
b, err := qconn.ReceiveDatagram(ctx)
if !errors.Is(err, expected) {
t.Fatal("not the error we expected", err)
}
Expand Down
12 changes: 8 additions & 4 deletions internal/netemx/http3.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,14 @@ func (srv *http3Server) mustListenPortLocked(handler http.Handler, ipAddr net.IP
}
go srvr.Serve(listener)

// make sure we track and close the listener: assuming the server was closing the
// listener seems to be the root cause of https://github.com/ooni/probe/issues/2527
// and closing the listener completely fixes the issue.
srv.closers = append(srv.closers, listener)
// For quic-go < 0.40.0, we needed the following fix as documented by the
// https://github.com/ooni/probe/issues/2527 issue.
//
// srv.closers = append(srv.closers, listener)
//
// After upgrading to quic-go/[email protected] (https://github.com/ooni/probe-cli/pull/1428)
// the above fix actually makes the code hang forever. We want to keep this message
// around for a couple of cycles (say until ooni/probe-cli < 3.22).

// make sure we track the server
srv.closers = append(srv.closers, srvr)
Expand Down

0 comments on commit 29a5384

Please sign in to comment.