net/http: http2 round tripper nil pointer dereference causes panic causing deadlock #65927
Comments
|
The client would set up separate goroutines for reading and writing, this can happen in theory when some error occurred in the write goroutine and it closed the the pipe while the read goroutine reading it. That's my guess for this, could you provide a reproducible demo for us to diagnose more deeply? |
panjf2000
added
WaitingForInfo
|
CC @neild |
|
@panjf2000 The scenario you are proposing makes a lot of sense with what I am seeing. I wish I could provide a reproducible example, so far I have not been able to recreate it locally, but it inevitably appears on production systems after some time. I am still trying to put something together to trigger the issue locally for debugging though. If it helps, the environment that my code is running has the following features:
So far, I have not correlated these kinds of protocol errors with the times at which the deadlock occurs, so I doubt it is related but thought it worth mentioning. I am in the process of adding additional instrumentation to my application in production in order to collect more details, I will report back if I find anything interesting or manage to recreate the issue locally. |
After a deeper look at the source code, my previous assumption may not stand. If that was the case, the Lines 3748 to 3750 in 83da21f However, I'm pretty sure that this issue had something to do with the fact that |
|
Change https://go.dev/cl/566675 mentions this issue: |
|
Change https://go.dev/cl/567175 mentions this issue: |
|
One possible path for this (possibly the only path? I haven't found any others): Server sends a 1xx response, followed by a DATA frame. https://go.googlesource.com/net/+/refs/tags/v0.21.0/http2/transport.go#2687 |
For golang/go#65927 Change-Id: I6f48706156384e026968cf9a6d9e0ec76b46fabf Reviewed-on: https://go-review.googlesource.com/c/net/+/566675 Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: Carlos Amedee <carlos@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
|
Change https://go.dev/cl/569819 mentions this issue: |
|
I've been running in production with the patch from https://go.dev/cl/566675 applied to the stdlib for about 8 days now and many million HTTP requests later I have not seen the deadlock issue pop up at all, so that appears to fix the issue. I am not sure about the patch in https://go.dev/cl/567175 but if needed I can apply it as well (either on its own or with the other one) to test it. |
|
@niallnsec Thanks, that gives me some added confidence that we've correctly identified the root cause. Historically, we haven't backported HTTP/2 fixes that can be applied by importing @gopherbot please open backport issues. This is an HTTP/2 bug with no good workaround. |
|
Backport issue(s) opened: #66254 (for 1.21), #66255 (for 1.22). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases. |
For #65785 #65927 Change-Id: I21791d4e22ae3039144f6b105ac439877f8b01bf Reviewed-on: https://go-review.googlesource.com/c/go/+/569819 Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: David Chase <drchase@google.com> Auto-Submit: Emmanuel Odeke <emmanuel@orijtech.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Commit-Queue: Emmanuel Odeke <emmanuel@orijtech.com> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
|
Change https://go.dev/cl/574855 mentions this issue: |
|
Change https://go.dev/cl/574875 mentions this issue: |
dmitshur
added
NeedsFix
…nd before final headers When checking to see if a DATA frame can be accepted, check to see if we have received a non-1xx header, not whether we have received any header. For golang/go#65927 Fixes golang/go#66255 Change-Id: Id4fae1862de6179f8fc95e02dec7d4c47a7640e1 Reviewed-on: https://go-review.googlesource.com/c/net/+/567175 Reviewed-by: Jonathan Amsterdam <jba@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-on: https://go-review.googlesource.com/c/net/+/574875 Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
…nd before final headers When checking to see if a DATA frame can be accepted, check to see if we have received a non-1xx header, not whether we have received any header. For golang/go#65927 Fixes golang/go#66254 Change-Id: Id4fae1862de6179f8fc95e02dec7d4c47a7640e1 Reviewed-on: https://go-review.googlesource.com/c/net/+/567175 Reviewed-by: Jonathan Amsterdam <jba@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-on: https://go-review.googlesource.com/c/net/+/574855 Auto-Submit: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
|
Change https://go.dev/cl/574935 mentions this issue: |
|
Change https://go.dev/cl/574916 mentions this issue: |
Pulls in one HTTP/2 fix: 0b0455d2c9 http2: reject DATA frames after 1xx and before final headers For #65927 Fixes #66254 Change-Id: I257b2634f63e8c6039c44dea24c345043c23c8d2 Reviewed-on: https://go-review.googlesource.com/c/go/+/574916 Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Pulls in one HTTP/2 fix: ae3c50b55f http2: reject DATA frames after 1xx and before final headers For #65927 Fixes #66255 Change-Id: Ib810455297083fc0722a997d0aa675132c38393c Reviewed-on: https://go-review.googlesource.com/c/go/+/574935 Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> TryBot-Bypass: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Pulls in one HTTP/2 fix: 0b0455d2c9 http2: reject DATA frames after 1xx and before final headers For golang/go#65927 Fixes golang/go#66254 Change-Id: I257b2634f63e8c6039c44dea24c345043c23c8d2 Reviewed-on: https://go-review.googlesource.com/c/go/+/574916 Reviewed-by: Than McIntosh <thanm@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Go version
go version go1.22.0 darwin/arm64
Output of
go envin your module/workspace:What did you do?
Issue occurs intermittently in a highly concurrent program making HTTP requests using Go's http.Client. The issue manifests as a deadlock in the program.
What did you see happen?
Looking specifically at the stack for
goroutine 2377it appears that there is a panic here:go/src/net/http/h2_bundle.go
Line 3751 in 6cbe522
which occurs as a result of p.b being nil. It looks like the deadlock occurs because as the stack unwinds and the deferred functions are called, cleanup() attempts to acquire a mutex which is already held further up the stack here:
go/src/net/http/h2_bundle.go
Line 9733 in 6cbe522
What did you expect to see?
The HTTP2 round tripper should not panic and cause a deadlock.
The text was updated successfully, but these errors were encountered: