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

Handle stream cancellation while outgoing data is buffered #2237

Merged
merged 3 commits into from Oct 11, 2018

Conversation

Projects
None yet
3 participants
@johanandren
Copy link
Member

johanandren commented Oct 2, 2018

Fixes #2236

@akka-ci akka-ci added validating tested and removed validating labels Oct 2, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Oct 2, 2018

Test PASSed.


expectNoBytes()

sendWINDOW_UPDATE(TheStreamId, 10000) // > than the remaining bytes (70000 - InitialWindowSize)

This comment has been minimized.

@johanandren

johanandren Oct 2, 2018

Member

was a bit surprising that the demuxer state didn't reach the WaitingForConnectionWindow until here, I'd thought it would get their already with the top sending of more than window bytes. However - I enabled tracing and it didn't happen until I repeated this much of the other window related test case.

This comment has been minimized.

@jrudolph

jrudolph Oct 11, 2018

Member

Hmm, the test didn't fail before the fix, should it?

This comment has been minimized.

@jrudolph

jrudolph Oct 11, 2018

Member

Any good observation, something smells like a bug here.

This comment has been minimized.

@jrudolph

jrudolph Oct 11, 2018

Member

Ah, wait makes sense, of course (but I didn't explain it when we talked about it): there's both connection-level and stream-level windows. WaitingForConnectionWindow is only reached if a stream is allowed to send (i.e. stream-level window is big enough) but nothing can be sent on the connection level. That's why you need that extra WINDOW_UPDATE which works on the stream level to get into that state.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Oct 2, 2018

Test PASSed.

@jrudolph
Copy link
Member

jrudolph left a comment

Changes are good, the WINDOW_UPDATE test doesn't seem to fail for me without the fix, though.

def closeStream(outStream: OutStream): Unit = {
if (sendableOutstreams.contains(outStream.streamId)) {
val sendableExceptClosed = sendableOutstreams - outStream.streamId
become(copy(sendableOutstreams = sendableExceptClosed))

This comment has been minimized.

@jrudolph

jrudolph Oct 11, 2018

Member

Ah, empty is fine here because there's still a control frame to send 👍

}

abstract class WithSendableOutStreams extends MultiplexerState {
def sendableOutstreams: immutable.Set[Int]
def withSendableOutstreams(sendableOutStreams: immutable.Set[Int]): WithSendableOutStreams

This comment has been minimized.

@jrudolph

expectNoBytes()

sendWINDOW_UPDATE(TheStreamId, 10000) // > than the remaining bytes (70000 - InitialWindowSize)

This comment has been minimized.

@jrudolph

jrudolph Oct 11, 2018

Member

Hmm, the test didn't fail before the fix, should it?


expectNoBytes()

sendWINDOW_UPDATE(TheStreamId, 10000) // > than the remaining bytes (70000 - InitialWindowSize)

This comment has been minimized.

@jrudolph

jrudolph Oct 11, 2018

Member

Any good observation, something smells like a bug here.

@jrudolph

This comment has been minimized.

Copy link
Member

jrudolph commented Oct 11, 2018

I added small improvements to the tests. Fix is good.

sendRST_STREAM(TheStreamId, ErrorCode.CANCEL)

entityDataOut.expectCancellation()
toNet.expectNoBytes() // the whole stage failed with bug #2236

This comment has been minimized.

@jrudolph

jrudolph Oct 11, 2018

Member

Actually with the bug, this just failed because the whole stage had failed because of the assertion for me.

@jrudolph jrudolph force-pushed the johanandren:wip-2236-http2-assertion-fail-on-cancellation-johanandren branch from c18761c to a9c5a34 Oct 11, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Oct 11, 2018

Test PASSed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Oct 11, 2018

Test PASSed.

@jrudolph
Copy link
Member

jrudolph left a comment

LGTM, thanks @johanandren.

@jrudolph jrudolph merged commit 5a77c29 into akka:master Oct 11, 2018

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@johanandren johanandren deleted the johanandren:wip-2236-http2-assertion-fail-on-cancellation-johanandren branch Oct 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment