internal/jsonrpc2_v2: fix a potential deadlock when (*Conn).Close is invoked during Bind
This fixes the goroutine leak reported in
https://build.golang.org/log/ae36d36843ca240e9e080886417a8798dd4c9618.
Fixes golang/go#46047 (hopefully for real this time).
Change-Id: I360e54d819849a35284c61d3a0655cc175d81f77
Reviewed-on: https://go-review.googlesource.com/c/tools/+/448095
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
diff --git a/internal/jsonrpc2_v2/conn.go b/internal/jsonrpc2_v2/conn.go
index 085e775..60afa70 100644
--- a/internal/jsonrpc2_v2/conn.go
+++ b/internal/jsonrpc2_v2/conn.go
@@ -82,6 +82,7 @@
// Connection.
type inFlightState struct {
connClosing bool // true when the Connection's Close method has been called
+ reading bool // true while the readIncoming goroutine is running
readErr error // non-nil when the readIncoming goroutine exits (typically io.EOF)
writeErr error // non-nil if a call to the Writer has failed with a non-canceled Context
@@ -140,14 +141,13 @@
s.closeErr = s.closer.Close()
s.closer = nil // prevent duplicate Close calls
}
- if s.readErr == nil {
+ if s.reading {
// The readIncoming goroutine is still running. Our call to Close should
// cause it to exit soon, at which point it will make another call to
- // updateInFlight, set s.readErr to a non-nil error, and mark the
- // Connection done.
+ // updateInFlight, set s.reading to false, and mark the Connection done.
} else {
- // The readIncoming goroutine has exited. Since everything else is idle,
- // we're completely done.
+ // The readIncoming goroutine has exited, or never started to begin with.
+ // Since everything else is idle, we're completely done.
if c.onDone != nil {
c.onDone()
}
@@ -240,10 +240,18 @@
reader := framer.Reader(rwc)
c.updateInFlight(func(s *inFlightState) {
+ select {
+ case <-c.done:
+ // Bind already closed the connection; don't start a goroutine to read it.
+ return
+ default:
+ }
+
// The goroutine started here will continue until the underlying stream is closed.
//
// (If the Binder closed the Connection already, this should error out and
// return almost immediately.)
+ s.reading = true
go c.readIncoming(ctx, reader, options.Preempter)
})
return c
@@ -514,6 +522,7 @@
}
c.updateInFlight(func(s *inFlightState) {
+ s.reading = false
s.readErr = err
// Retire any outgoing requests that were still in flight: with the Reader no
