Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/http2: DATA frame transmission failure because of race condition #12998

Closed
tatsuhiro-t opened this issue Oct 20, 2015 · 3 comments
Closed

x/net/http2: DATA frame transmission failure because of race condition #12998

tatsuhiro-t opened this issue Oct 20, 2015 · 3 comments

Comments

@tatsuhiro-t
Copy link

@tatsuhiro-t tatsuhiro-t commented Oct 20, 2015

We found the bug in x/net/http2 module which causes DATA frame transmission failure due to race condition.
The bug was discovered when h2load is used against caddy web server.
h2load is HTTP/2 aware load tool, and caddy is web server which uses x/net/http2.
nghttp2/nghttp2#397

I have a patch to fix this problem. I'll upload a patch to code review.

@tatsuhiro-t

This comment has been minimized.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 20, 2015

CL https://golang.org/cl/16037 mentions this issue.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 20, 2015

CL https://golang.org/cl/16063 mentions this issue.

@golang golang locked and limited conversation to collaborators Oct 24, 2016
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
This changes makes sure we never write to *writeData in the ServeHTTP
goroutine until the serve goroutine is done with it.

Also, it makes sure we don't transition the stream to the closed state
on the final DATA frame concurrently with the write.

To fix both, the writeFrameAsync goroutine no longer replies directly back
to the ServeHTTP goroutine with the write result. It's now passed to
the serve goroutine instead, which looks at the frameWriteMsg to
decide how to advance the state machine, then signals the ServeHTTP
goroutine with the result, and then advances the state machine.

Because advancing the state machine could transition it to closed,
which the ServeHTTP goroutine might also be selecting on, make the
ServeHTTP goroutine prefer its frameWriteMsg response channel for errors
over the stream closure in its select.

Various code simplifications and robustness in the process.

Tests now pass reliably even with high -count values, -race on/off,
etc. I've been unable to make h2load be unhappy now either.

Thanks to Tatsuhiro Tsujikawa (Github user @tatsuhiro-t) for the bug
report and debugging clues.

Fixes golang/go#12998

Change-Id: I441c4c9ca928eaba89fd4728d213019606edd899
Reviewed-on: https://go-review.googlesource.com/16063
Reviewed-by: Andrew Gerrand <adg@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.