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

Jump to bottom

http2: 'pull' was lost when closing while waiting for window #3672

Merged
merged 2 commits into from Dec 3, 2020
Merged

http2: 'pull' was lost when closing while waiting for window #3672

merged 2 commits into from Dec 3, 2020

Conversation

raboof
Copy link
Member

@raboof raboof commented Nov 26, 2020 •
edited

on top of #3671 (rebased)

@akka-ci akka-ci added the validating PR that is currently being validated by Jenkins label Nov 26, 2020
@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Nov 26, 2020
@akka-ci
Copy link

akka-ci commented Nov 26, 2020

Test PASSed.

@raboof raboof requested a review from ignasi35 November 27, 2020 10:59
@raboof raboof mentioned this pull request Nov 27, 2020
Closed
@jrudolph
Copy link
Member

jrudolph commented Dec 3, 2020

Needs a rebase

@jrudolph
Copy link
Member

jrudolph commented Dec 3, 2020

Seemed it was easy enough to merge/resolve conflicts.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Dec 3, 2020
@raboof raboof force-pushed the lost-pull-when-closing-while-waiting-for-window branch from 48c56b8 to 1d52c94 Compare December 3, 2020 10:30
jrudolph
jrudolph approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member

@jrudolph jrudolph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great catch, so the effect was that the connection was somewhat stalled after that happened?

@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins and removed validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Dec 3, 2020
@jrudolph
Copy link
Member

jrudolph commented Dec 3, 2020

Hmm not sure the new scenario is done yet? Should we just merge the original fix or is there a problem with it?

@raboof
Copy link
Member Author

raboof commented Dec 3, 2020

LGTM, great catch, so the effect was that the connection was somewhat stalled after that happened?

Yes, the test added in this PR would time out instead of continue (though I don't remember exactly at which step)

@raboof
Copy link
Member Author

raboof commented Dec 3, 2020

Hmm not sure the new scenario is done yet? Should we just merge the original fix or is there a problem with it?

I think we need additional fixes from #3673 to complete this scenario (so it would be good to merge this and follow-up with that), but I'll double-check

@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Dec 3, 2020
@akka-ci
Copy link

akka-ci commented Dec 3, 2020

Test PASSed.

@raboof raboof force-pushed the lost-pull-when-closing-while-waiting-for-window branch from 1d52c94 to a0b86d0 Compare December 3, 2020 10:59
raboof
raboof commented Dec 3, 2020
View reviewed changes
akka-http2-support/src/test/scala/akka/http/impl/engine/http2/Http2ClientSpec.scala

// TODO the client stack should not accept new connections anymore
// TODO what to do with requests that are already in the queue at this point?
// user.requestOut.expectCancellation()
Copy link
Member Author

@raboof raboof Dec 3, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This indeed needs further changes before it can be enabled in the test, which are coming in #3673 - so OK to merge without this.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Dec 3, 2020
@akka-ci akka-ci removed the validating PR that is currently being validated by Jenkins label Dec 3, 2020
@akka-ci
Copy link

akka-ci commented Dec 3, 2020

Test PASSed.

@raboof raboof merged commit ab38d36 into akka:master Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrudolph jrudolph jrudolph approved these changes

@johanandren johanandren johanandren approved these changes

@ignasi35 ignasi35 Awaiting requested review from ignasi35

Assignees
No one assigned
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@raboof @akka-ci @jrudolph @johanandren