PROTON-2790: finer grained session flow control #434
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
astitcher
reviewed
Sep 16, 2024
There was a problem hiding this comment.
I'm not able to review this thoroughly as I don't have time to really understand everything the code is doing.
However it looks good to me.
The only obvious thing is that instead of silently disregarding a request it would be better to log the error/warning if you can't return an error.
| @@ -2828,6 +2861,8 @@ uint32_t pn_transport_get_max_frame(pn_transport_t *transport) | |||
| void pn_transport_set_max_frame(pn_transport_t *transport, uint32_t size) | |||
| { | |||
| // if size == 0, no advertised limit to input frame size. | |||
| if (transport->open_sent) | |||
| return; // Too late. Silently disregard request. | |||
There was a problem hiding this comment.
Instead of silently disregarding the request it's probably better to log a warning.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch provides an implementation of PROTON-2790 to provide finer grained management of session based flow control over input buffering.
In addition to limiting the absolute size of the inbound buffered data, this patch allows the application to choose how soon the peer is notified of new available space (the existing method waits until the capacity is fully used up which can result in needless transmission stalls). This is particularly useful for large messages or streaming messages which are broken into multiple AMQP transfer frames.
The patch also allows the sending peer the ability to query what is the most recently communicated window size (pn_session_remote_incoming_window).
This patch also prevents anti-patterns that are possible in the current session flow model that can lead to PN_TRANSPORT_ERROR termination of the connection. Namely the transport maximum frame size and the session incoming window will not be allowed to change at arbitrary times (which can otherwise allow the incoming window to become negative). Session "capacity" can still be changed at any time to preserve existing usage for applications that may rely on this API (now deprecated).
This patch strives to send the least number of additional AMQP frames on the wire to achieve the intended result. No extra frames are sent if link credit flow frames are already pending on the session or if the incoming window is currently at or above the low water mark.
The existing session flow model based on byte capacity is marked as deprecated in favor of this alternative flow control model. The code is structured to allow the easy removal of the "capacity" model in future.