Fix occasional hang with http2 goaway frames #3516
Conversation
Http2Connection was dropping headers of healthy streams after GOAWAY frame. Fixes square#3422 Signed-off-by: Igor Fedorenko <[email protected]>
|
Not sure why Travis build failed, all tests passed in Travis and |
|
Restarted build. The VM crashed for an unknown reason.
…On Thu, Aug 10, 2017, 6:17 AM ifedorenko ***@***.***> wrote:
Not sure why Travis build failed, all tests passed in Travis and mvn
clean verify works for me locally too (osx, java 8)
Results :
Tests run: 350, Failures: 0, Errors: 0, Skipped: 6
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3516 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAEEEZ-S1EIyklxU9LJh4E4pCmvtIHWfks5sWwL8gaJpZM4OzYqT>
.
|
| @@ -620,7 +620,7 @@ public Http2Connection build() { | |||
| // Create a stream. | |||
| final Http2Stream newStream = new Http2Stream(streamId, Http2Connection.this, | |||
| false, inFinished, headerBlock); | |||
| lastGoodStreamId = streamId; | |||
| if (streamId > lastGoodStreamId) lastGoodStreamId = streamId; | |||
There was a problem hiding this comment.
I don’t think this works. The lastGoodStreamId refers to the local last good stream, and this is the remote last good stream
There was a problem hiding this comment.
Actually, scratch that. In this case both refer to remote stream IDs.
| @@ -725,6 +725,7 @@ private void applyAndAckSettings(final Settings peerSettings) { | |||
| synchronized (Http2Connection.this) { | |||
| streamsCopy = streams.values().toArray(new Http2Stream[streams.size()]); | |||
| shutdown = true; | |||
| Http2Connection.this.lastGoodStreamId = lastGoodStreamId; | |||
There was a problem hiding this comment.
could this cause the stream ID to become smaller?
There was a problem hiding this comment.
I think this is the line that’s most problematic. The local field lastGoodStreamId refers to remotely-initiated streams, and goAway is telling us an ID for locally-initiated streams
| @@ -978,6 +982,33 @@ private void noRecoveryFromErrorWithRetryDisabled(ErrorCode errorCode) throws Ex | |||
| assertEquals(0, server.takeRequest().getSequenceNumber()); | |||
| } | |||
|
|
|||
| @Test public void responseHeadersAfterGoaway() throws Exception { | |||
|
I’m trying to understand exactly what’s going on here, and I’m having a bit of trouble mostly because I haven’t looked at this code for a long time. Client sends request 1 I think the fix is to move this line in down into the block that runs only if the stream is unknown: |
|
... Just tested it, and that’s sufficient to get the new test to pass. If you’re cool with that, that’s what I’ll merge. |
|
Yes, this is the scenario that causes 3.8.1 to hang and I confirmed your fix solves the problem when running against nginx. Thank you for looking into this. |
|
Merged manually. Thanks! |
Http2Connection was dropping headers of healthy streams
after GOAWAY frame.
Fixes #3422