Skip to content

Commit

Permalink
quic: avoid deadlock in Endpoint.Close
Browse files Browse the repository at this point in the history
Don't hold Endpoint.connsMu while calling Conn methods that can
indirectly depend on acquiring it.

Also change test cleanup to not wait for connections to drain
when closing a test Endpoint, removing an unnecessary 0.1s delay
in test runtime.

Fixes golang/go#64982.

Change-Id: If336e63b0a7f5b8d2ef63986d36f9ee38a92c477
Reviewed-on: https://go-review.googlesource.com/c/net/+/554695
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
  • Loading branch information
neild committed Jan 8, 2024
1 parent cb5b10f commit 26b646e
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 6 deletions.
16 changes: 11 additions & 5 deletions internal/quic/endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,25 +103,31 @@ func (e *Endpoint) LocalAddr() netip.AddrPort {
// It waits for the peers of any open connection to acknowledge the connection has been closed.
func (e *Endpoint) Close(ctx context.Context) error {
e.acceptQueue.close(errors.New("endpoint closed"))

// It isn't safe to call Conn.Abort or conn.exit with connsMu held,
// so copy the list of conns.
var conns []*Conn
e.connsMu.Lock()
if !e.closing {
e.closing = true
e.closing = true // setting e.closing prevents new conns from being created
for c := range e.conns {
c.Abort(localTransportError{code: errNo})
conns = append(conns, c)
}
if len(e.conns) == 0 {
e.udpConn.Close()
}
}
e.connsMu.Unlock()

for _, c := range conns {
c.Abort(localTransportError{code: errNo})
}
select {
case <-e.closec:
case <-ctx.Done():
e.connsMu.Lock()
for c := range e.conns {
for _, c := range conns {
c.exit()
}
e.connsMu.Unlock()
return ctx.Err()
}
return nil
Expand Down
2 changes: 1 addition & 1 deletion internal/quic/endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func newLocalEndpoint(t *testing.T, side connSide, conf *Config) *Endpoint {
t.Fatal(err)
}
t.Cleanup(func() {
e.Close(context.Background())
e.Close(canceledContext())
})
return e
}
Expand Down

0 comments on commit 26b646e

Please sign in to comment.