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

httpsession: simplify queue processing logic #48109

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jkarneges
Copy link
Member

@jkarneges jkarneges commented Jan 31, 2025

Currently, HttpSession sends messages to clients while in either the Holding or SendingQueue states. Sending during the Holding state happens when the session is idle and a message is published to it. Sending during the SendingQueue state happens when the session starts sending messages that had queued up while it was in some state other than Holding or SendingQueue. The SendingQueue state can be thought of as a sort of catch-up state.

Having two distinct states for sending messages makes the code hard to follow, and upon review there doesn't seem to be much difference between the way the two states are meant to work. Notably, if the client can't be written to while in Holding state, a queue of messages still builds up. If anything, having two states for the same thing has resulted in subtle inconsistencies and bugs, especially after we introduced async message filters which made sending non-atomic. This PR consolidates on SendingQueue as the one state for sending messages, and fixes a few related issues.

The changes:

  • Holding state is now only for idleness. If a message is published to the session while in that state, it switches to SendingQueue to process the message (as well as any others that arrive during processing). After all the messages in the queue have been processed, the state switches back to Holding.
  • The keep alive timer is now always disabled when the client can't be written to. Previously this was only happening while in the Holding state, but not the SendingQueue state, which was probably not intended.
  • The keep alive timer (and next link timeout, via updatesManager registration) are always restored once the client is able to be written to again. Previously, this restoration would only occur when in the SendingQueue state (meaning, backpressure while in Holding state broke these timers). This is now done in sendQueueDone(), with care taken to not reset the timeouts if they are already active.
  • Response mode now doesn't get stuck if a filter drops a message. Response mode is only able to send exactly 1 message to the client, and normally this processing sets the state to Closing, but if the message is dropped then the state gets stuck as SendingQueue. Now we call sendQueueDone() after processing all messages, just like stream mode, which ensures the state gets set back to Holding so another message can be accepted.
  • Related to the above, response mode now doesn't discard new messages while it is in the middle of processing a message. This is important because, even though response mode can only send one message, a filter could end up dropping the current message in which case we still want to process subsequent ones.

Testing: manually tested all the changed code paths.

@jkarneges jkarneges force-pushed the jkarneges/httpsession-fix-state branch from 12b90f5 to e2f6ade Compare January 31, 2025 19:56
@jkarneges jkarneges force-pushed the jkarneges/httpsession-fix-state branch from e2f6ade to 164cdc1 Compare February 1, 2025 02:56
@jkarneges jkarneges marked this pull request as ready for review February 1, 2025 02:57
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.

1 participant