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

Correctly propagate failures while update the flow-controller to the … #9664

Merged
merged 3 commits into from Oct 22, 2019

Conversation

@normanmaurer
Copy link
Member

normanmaurer commented Oct 14, 2019

…multiplexed channel

Motivation:

We may fail to update the flow-controller and in this case need to notify the stream channel and close it.

Modifications:

Attach a future to the write of the update frame and in case of a failure propagate it to the channel and close it

Result:

Fixes #9663

…multiplexed channel

Motivation:

We may fail to update the flow-controller and in this case need to notify the stream channel and close it.

Modifications:

Attach a future to the write of the update frame and in case of a failure propagate it to the channel and close it

Result:

Fixes #9663
@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 14, 2019

@ti2ger92 PTAL

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 14, 2019

This still needs a test case....

@normanmaurer normanmaurer requested a review from carl-mastrangelo Oct 16, 2019
@normanmaurer normanmaurer added this to the 4.1.43.Final milestone Oct 16, 2019
@bryce-anderson

This comment has been minimized.

Copy link
Contributor

bryce-anderson commented Oct 16, 2019

This seems fine to me but I'm having a hard time figuring out why it solves the core issue in #9663 which is that it appears we've 'processed' more bytes than we received.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 16, 2019

@bryce-anderson I am still trying to understand that as well 🤦‍♂

@ti2ger92

This comment has been minimized.

Copy link

ti2ger92 commented Oct 17, 2019

Sorry for the delay, but I was able to get the latest branch imported and rerun. It doesn't seem to resolve our issue, a WINDOW_UPDATE is still not sent back.

@normanmaurer normanmaurer force-pushed the window_update_failure branch from 7798d13 to dd7f6ae Oct 21, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 21, 2019

@bryce-anderson alright ... I found out why it fixed the problem and re-factored to still batch the window update writes. The problem was some re-entrancy problem. See the comments in the code.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 21, 2019

@ti2ger92 please provide a standalone reproducer (netty only). Keep in mind that window-updates will only be send when really needed (the window was used almost completely ).

@ti2ger92

This comment has been minimized.

Copy link

ti2ger92 commented Oct 21, 2019

@normanmaurer , when the change you made is applied to our system, our use case seems OK. I think you fixed it 🎉🎊🎉🏁🎉

Can you clarify how the gist I uploaded should be reduced further?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 21, 2019

@ti2ger92 a testcase / reproducer that only uses netty would be nice :)

@normanmaurer normanmaurer merged commit 9bf10f2 into 4.1 Oct 22, 2019
3 checks passed
3 checks passed
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer normanmaurer deleted the window_update_failure branch Oct 22, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Oct 22, 2019

@ti2ger92 thanks for testing it

normanmaurer added a commit that referenced this pull request Oct 22, 2019
#9664)


Motivation:

We may fail to update the flow-controller and in this case need to notify the stream channel and close it.

Modifications:

Attach a future to the write of the update frame and in case of a failure propagate it to the channel and close it

Result:

Fixes #9663
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.