Skip to content

Commit 8d10596

Browse files
neildgopherbot
neild
authored and
gopherbot
committed
http2: avoid deadlocks in wrapped ClientConn state callback
An http2.ClientConn wraps an http.ClientConn. The http.ClientConn calls back to the http2.ClientConn's state hook on certain state changes. Since the state hook acquires the http2.ClientConn's mutex, holding the mutex while calling an http.ClientConn method which can trigger a state hook call can cause a deadlock. Avoid deadlocks in two places where the http2.ClientConn held its mutex while calling http.ClientConn.Reserve. Fixes golang/go#80680 Change-Id: I9b4107e1461d451cdb2eae7b102bd9ba6a6a6964 Reviewed-on: https://go-review.googlesource.com/c/net/+/808900 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Nicholas Husin <husin@google.com> Reviewed-by: Nicholas Husin <nsh@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> Auto-Submit: Damien Neil <dneil@google.com>
1 parent 99c3b0a commit 8d10596

2 files changed

Lines changed: 123 additions & 18 deletions

File tree

‎http2/transport_api_test.go‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -770,6 +770,98 @@ func TestAPIClientConnPing(t *testing.T) {
770770
})
771771
}
772772

773+
// TestAPITransportClientConnReserveNewRequest tests the ClientConn ReserveNewRequest method.
774+
func TestAPITransportClientConnReserveNewRequest(t *testing.T) {
775+
synctest.Test(t, func(t *testing.T) {
776+
tt := newTestTransport(t, roundTripXNetHTTP2)
777+
778+
nc := tt.li.newConn()
779+
cc, err := tt.tr.NewClientConn(nc)
780+
if err != nil {
781+
t.Fatalf("NewClientConn: %v", err)
782+
}
783+
784+
tc1 := tt.getConn()
785+
tc1.wantFrameType(http2.FrameSettings)
786+
tc1.wantFrameType(http2.FrameWindowUpdate)
787+
tc1.writeSettings(http2.Setting{
788+
ID: http2.SettingMaxConcurrentStreams,
789+
Val: 1,
790+
})
791+
tc1.wantFrameType(http2.FrameSettings) // ACK
792+
793+
synctest.Wait()
794+
wantClientConnState(t, cc.State(), http2.ClientConnState{
795+
MaxConcurrentStreams: 1,
796+
})
797+
798+
if got, want := cc.ReserveNewRequest(), true; got != want {
799+
t.Fatalf("cc.ReserveNewRequest() = %v, want %v", got, want)
800+
}
801+
wantClientConnState(t, cc.State(), http2.ClientConnState{
802+
MaxConcurrentStreams: 1,
803+
StreamsReserved: 1,
804+
})
805+
806+
// Issue #80680: Deadlock when ReserveNewRequest invokes the state hook.
807+
go func() {
808+
for i := range 100 {
809+
tc1.writeSettings(http2.Setting{
810+
ID: http2.SettingMaxConcurrentStreams,
811+
Val: uint32(i * 2),
812+
})
813+
}
814+
}()
815+
go func() {
816+
for range 100 {
817+
cc.ReserveNewRequest()
818+
}
819+
}()
820+
synctest.Wait()
821+
})
822+
}
823+
824+
func TestAPITransportSettingsRoundTripRace(t *testing.T) {
825+
synctest.Test(t, func(t *testing.T) {
826+
tt := newTestTransport(t, roundTripXNetHTTP2)
827+
828+
nc := tt.li.newConn()
829+
cc, err := tt.tr.NewClientConn(nc)
830+
if err != nil {
831+
t.Fatalf("NewClientConn: %v", err)
832+
}
833+
834+
tc1 := tt.getConn()
835+
tc1.wantFrameType(http2.FrameSettings)
836+
tc1.wantFrameType(http2.FrameWindowUpdate)
837+
tc1.writeSettings(http2.Setting{
838+
ID: http2.SettingMaxConcurrentStreams,
839+
Val: 1,
840+
})
841+
tc1.wantFrameType(http2.FrameSettings) // ACK
842+
synctest.Wait()
843+
844+
go func() {
845+
for i := range 100 {
846+
tc1.writeSettings(http2.Setting{
847+
ID: http2.SettingMaxConcurrentStreams,
848+
Val: uint32(i * 2),
849+
})
850+
}
851+
}()
852+
go func() {
853+
for range 100 {
854+
req, _ := http.NewRequest("GET", "http://example.tld/", nil)
855+
resp, err := cc.RoundTrip(req)
856+
if err == nil {
857+
resp.Body.Close()
858+
}
859+
}
860+
}()
861+
synctest.Wait()
862+
})
863+
}
864+
773865
// TestAPIClientConnShutdown tests the ClientConn.Shutdown method.
774866
// Shutdown returns after the last request on the connection completes.
775867
func TestAPIClientConnShutdownSuccess(t *testing.T) {

‎http2/transport_wrap.go‎

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -237,32 +237,40 @@ type ClientConn struct {
237237
}
238238

239239
func (cc *ClientConn) roundTrip(req *http.Request) (*http.Response, error) {
240-
err := func() error {
240+
haveReservation, err := func() (bool, error) {
241241
cc.mu.Lock()
242242
defer cc.mu.Unlock()
243243
if cc.doNotReuse {
244-
return errClientConnUnusable
245-
}
246-
cc.roundTrips++
247-
if cc.reserved > 0 {
248-
// We've already reserved a concurrency slot for this request.
249-
cc.reserved--
250-
} else if cc.cc.Reserve() != nil {
251-
// We don't seem to have an available concurrency slot,
252-
// so bump the pending count (requests waiting for a slot).
253-
cc.pending++
244+
return false, errClientConnUnusable
254245
}
246+
255247
// ClientConn.Shutdown will not shut down the conn while
256248
// cc.starting > 0 or cc.cc.InFlight() > 0.
257249
//
258250
// The starting state covers the gap between us deciding to
259251
// start sending the request, and actually sending it.
260252
cc.starting++
261-
return nil
253+
254+
cc.roundTrips++
255+
if cc.reserved == 0 {
256+
// We do not have a concurrency slot reserved for this request.
257+
return false, nil
258+
}
259+
cc.reserved--
260+
return true, nil
262261
}()
263262
if err != nil {
264263
return nil, err
265264
}
265+
// If we have no reservation, try to acquire one.
266+
// (This must be done without cc.mu held, since Reserve may call back to the state hook.)
267+
if !haveReservation && cc.cc.Reserve() != nil {
268+
// We could not acquire a concurrency slot, so bump the pending count
269+
// (requests waiting for a slot).
270+
cc.mu.Lock()
271+
cc.pending++
272+
cc.mu.Unlock()
273+
}
266274
resp, err := cc.cc.RoundTrip(req)
267275
cc.mu.Lock()
268276
cc.starting--
@@ -293,16 +301,21 @@ func (cc *ClientConn) ping(ctx context.Context) error {
293301
}
294302

295303
func (cc *ClientConn) reserveNewRequest() bool {
304+
if err := cc.cc.Reserve(); err != nil {
305+
return false
306+
}
307+
reserved := true
296308
cc.mu.Lock()
297-
defer cc.mu.Unlock()
298309
if cc.doNotReuse {
299-
return false
310+
reserved = false
311+
} else {
312+
cc.reserved++
300313
}
301-
if err := cc.cc.Reserve(); err != nil {
302-
return false
314+
cc.mu.Unlock()
315+
if !reserved {
316+
cc.cc.Release()
303317
}
304-
cc.reserved++
305-
return true
318+
return reserved
306319
}
307320

308321
func (cc *ClientConn) setDoNotReuse() {

0 commit comments

Comments
 (0)