-
Notifications
You must be signed in to change notification settings - Fork 545
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
fix: interceptor back-pressure #3374
Conversation
6950c88
to
4e3cfc5
Compare
Not working yet, just a POC |
@@ -517,6 +529,11 @@ async function connect (client) { | |||
|
|||
function emitDrain (client) { | |||
client[kNeedDrain] = 0 | |||
|
|||
for (const callback of client[kDrainQueue].splice(0)) { |
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.
How does it differs from subscribing to dispatcher.on('drain')
?
Sure, it is integrated as first-class-citizen, but it kind overlaps with the drain
event isn't it?
If the retry
interceptor wants to implement backpressure, it can handle the last value returned from dispatch
, and decide what to do on the next attempt, e.g. waiting for the event or trying once more, isn't it?
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.
'drain' applies globally while onDrain applies to this specific request, e.g. you could have different origins that apply to different queues, so having a global drain is confusing.
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.
Which is why the current dispatch: boolean
+ drain 'event'
API is slightly broken.
Tries to resolve the back-pressure issue. Downside is that it gives up on forwarding dispatcher events.
Refs: #3376