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

fix: Client.stream writableNeedDrain #442

Merged
merged 8 commits into from
Nov 13, 2020
Merged

fix: Client.stream writableNeedDrain #442

merged 8 commits into from
Nov 13, 2020

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 25, 2020

@ronag ronag requested a review from mcollina September 25, 2020 16:51
@ronag
Copy link
Member Author

ronag commented Sep 25, 2020

Missing tests. WIP

@ronag ronag marked this pull request as draft September 25, 2020 16:51
@ronag ronag force-pushed the writable-need-drain branch 4 times, most recently from 1d3eed1 to 3cb9234 Compare September 25, 2020 16:58
@ronag ronag force-pushed the writable-need-drain branch 2 times, most recently from 68a5e14 to 0e03afc Compare October 10, 2020 12:38
@ronag ronag added the Status: help-wanted This issue/pr is open for contributions label Nov 2, 2020
@ronag ronag force-pushed the writable-need-drain branch 2 times, most recently from d86c660 to 03c41a5 Compare November 12, 2020 11:23
@ronag ronag marked this pull request as ready for review November 12, 2020 11:24
@ronag
Copy link
Member Author

ronag commented Nov 12, 2020

@mcollina: PTAL

@ronag
Copy link
Member Author

ronag commented Nov 12, 2020

@indutny @addaleax I'm working around the fact that I can't pause the parser in a callback. Do you have any better suggestions on how to go about that?

Copy link
Member

@mcollina mcollina left a 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).

@ronag
Copy link
Member Author

ronag commented Nov 12, 2020

I don't understand this fix. Why is this needed at all?

Because currently when pausing we don't actually pause and the parser might invoke more callbacks. It's more of a hint.

A slightly better implementation would go and replace the kOn* methods when paused to avoid the ifs on an hot path

I think this is negligable.

@ronag
Copy link
Member Author

ronag commented Nov 12, 2020

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 false from onBody is only a hint.

@mcollina
Copy link
Member

Have you benchmarked this?

@ronag
Copy link
Member Author

ronag commented Nov 13, 2020

Yes, difference is within margin of error (which is quite large, we should probably increase number of samples in benchmark again).

@ronag
Copy link
Member Author

ronag commented Nov 13, 2020

I've cleaned this up a bit and also added more assertions to make it clearer what this fixes.

test/client-stream.js Outdated Show resolved Hide resolved
test/client-stream.js Outdated Show resolved Hide resolved
@ronag ronag requested a review from mcollina November 13, 2020 08:27
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@ronag ronag merged commit 85f05ed into master Nov 13, 2020
@ronag ronag deleted the writable-need-drain branch November 13, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: help-wanted This issue/pr is open for contributions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

writableNeedDrain
2 participants