-
-
Notifications
You must be signed in to change notification settings - Fork 685
fix: Client.stream writableNeedDrain #442
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
|
Missing tests. WIP |
1d3eed1 to
3cb9234
Compare
68a5e14 to
0e03afc
Compare
0e03afc to
76010a2
Compare
d86c660 to
03c41a5
Compare
|
@mcollina: PTAL |
e2d3829 to
1f345b9
Compare
mcollina
left a comment
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 don't understand this fix. Why is this needed at all?
(A slightly better implementation would go and replace the kOn* methods when paused to avoid the ifs on an hot path).
Because currently when pausing we don't actually pause and the parser might invoke more callbacks. It's more of a hint.
I think this is negligable. |
|
I guess it's not entirely necessary... though the current implementation isn't strictly correct. The docs would need to be updated to reflect that returning |
|
Have you benchmarked this? |
|
Yes, difference is within margin of error (which is quite large, we should probably increase number of samples in benchmark again). |
e2568b0 to
a0d8cb0
Compare
a0d8cb0 to
7a40258
Compare
|
I've cleaned this up a bit and also added more assertions to make it clearer what this fixes. |
mcollina
left a comment
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.
LGTM
Fixes: #441
Refs: nodejs/node#35348
Refs: nodejs/node#35341