Skip to content

websocketoverhttp: support keeping back data until more arrives#48185

Merged
jkarneges merged 3 commits intomainfrom
jkarneges/woh-content-accepted
Apr 1, 2025
Merged

websocketoverhttp: support keeping back data until more arrives#48185
jkarneges merged 3 commits intomainfrom
jkarneges/woh-content-accepted

Conversation

@jkarneges
Copy link
Copy Markdown
Member

@jkarneges jkarneges commented Mar 28, 2025

Implements #48187. Basically, instead of calling removeContentFromFrames() before sending the exported events to the backend, it is called after receiving the response from the backend, and instead of removing all exported content only the amount accepted is removed.

Notes:

  • It is not possible to keep back a CLOSE message, since there won't be any data after it to wait for. If Pushpin sends a request containing a CLOSE, the backend must accept all of the content in the request or the session will be terminated.
  • The amount of data that can be kept back is limited to the size of Pushpin's buffer. If exceeded (as determined by the inability to make progress), the session will be terminated.
  • The buffer is always replayed in the next request, even if the next request is not caused by the arrival of more data from the client. Using Keep-Alive-Interval or publishing a refresh action will cause the buffer to be replayed.
  • When Pushpin replays data, it includes a Content-Bytes-Replayed header in the request. The backend can use this to confirm whether the use of Content-Bytes-Accepted worked.

@jkarneges jkarneges force-pushed the jkarneges/woh-content-accepted branch from 6629e18 to 7762971 Compare March 28, 2025 00:20
@jkarneges jkarneges changed the title websocketoverhttp: support replaying data after more arrives websocketoverhttp: support keeping back data until more arrives Mar 28, 2025
@jkarneges jkarneges force-pushed the jkarneges/woh-content-accepted branch from 7762971 to df7970c Compare March 31, 2025 02:03
Copy link
Copy Markdown
Contributor

@mhp mhp left a comment

Choose a reason for hiding this comment

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

I checked the various bits of bookkeeping and it looked right to me. How easy is it to add a couple of test cases, to make sure this continues to work?

@jkarneges
Copy link
Copy Markdown
Member Author

Tests for these changes won't be easy since they'll need to be async against a fake backend and we don't have any such tests like that for this class yet, but this is a good occasion to remedy that. Thanks for approving. I'll land and figure out how to add tests.

@jkarneges jkarneges merged commit 94efde4 into main Apr 1, 2025
19 checks passed
@jkarneges jkarneges deleted the jkarneges/woh-content-accepted branch April 1, 2025 16:35
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.

2 participants