-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[backend] SSE message backpressure mechanism #13909
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13909 +/- ##
==========================================
- Coverage 30.86% 30.79% -0.08%
==========================================
Files 2911 2912 +1
Lines 192486 193960 +1474
Branches 39265 39880 +615
==========================================
+ Hits 59419 59726 +307
- Misses 133067 134234 +1167
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d8f14ac to
45d702c
Compare
| if (res.finished || !res.writable) { | ||
| return; | ||
| } | ||
| // region build message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i prefer to use function to group the code
| logApp.debug('[STREAM] Buffer draining', { buffer: res.writableLength, limit: res.writableHighWaterMark }); | ||
| await once(res, 'drain'); | ||
| } | ||
| res.flush(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one flush per chunk is not necessary. should be done only at the end of all chunks
| const chunk = messageBuffer.subarray(offset, end); | ||
| if (!res.write(chunk)) { | ||
| logApp.debug('[STREAM] Buffer draining', { buffer: res.writableLength, limit: res.writableHighWaterMark }); | ||
| await once(res, 'drain'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if chunk is no more necessary, i prefer to remove it, since we can easily break the SSE message due to race conditions caused by this await. this race condition may already exist with heartbeat that are issued in //
|
Tested successfully. |
Proposed changes
Related issues
Checklist