Skip to content

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

Merged
normanmaurer merged 3 commits into4.1from
window_update_failure
Oct 22, 2019
Merged

Correctly propagate failures while update the flow-controller to the …#9664
normanmaurer merged 3 commits into4.1from
window_update_failure

Conversation

@normanmaurer
Copy link
Copy Markdown
Member

…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
Copy link
Copy Markdown
Member Author

@ti2ger92 PTAL

@normanmaurer
Copy link
Copy Markdown
Member Author

This still needs a test case....

@normanmaurer normanmaurer added this to the 4.1.43.Final milestone Oct 16, 2019
@bryce-anderson
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Member Author

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

@ti2ger92
Copy link
Copy Markdown

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 Compare October 21, 2019 07:01
@normanmaurer
Copy link
Copy Markdown
Member Author

@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
Copy link
Copy Markdown
Member Author

@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
Copy link
Copy Markdown

@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
Copy link
Copy Markdown
Member Author

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

@normanmaurer normanmaurer merged commit 9bf10f2 into 4.1 Oct 22, 2019
@normanmaurer normanmaurer deleted the window_update_failure branch October 22, 2019 12:40
@normanmaurer
Copy link
Copy Markdown
Member Author

@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

no WINDOW_UPDATE sent due to uncaught error in promise

5 participants