Skip to content

fix: Client.stream writableNeedDrain#442

Merged
ronag merged 8 commits into
masterfrom
writable-need-drain
Nov 13, 2020
Merged

fix: Client.stream writableNeedDrain#442
ronag merged 8 commits into
masterfrom
writable-need-drain

Conversation

@ronag

@ronag ronag commented Sep 25, 2020

Copy link
Copy Markdown
Member

@ronag ronag requested a review from mcollina September 25, 2020 16:51
@ronag

ronag commented Sep 25, 2020

Copy link
Copy Markdown
Member Author

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 force-pushed the writable-need-drain branch from 0e03afc to 76010a2 Compare November 2, 2020 13:52
@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

ronag commented Nov 12, 2020

Copy link
Copy Markdown
Member Author

@mcollina: PTAL

Comment thread lib/core/client.js
@ronag

ronag commented Nov 12, 2020

Copy link
Copy Markdown
Member Author

@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?

@ronag ronag force-pushed the writable-need-drain branch from e2d3829 to 1f345b9 Compare November 12, 2020 12:36

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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

ronag commented Nov 12, 2020

Copy link
Copy Markdown
Member Author

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

ronag commented Nov 12, 2020

Copy link
Copy Markdown
Member Author

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
Copy Markdown
Member

Have you benchmarked this?

@ronag

ronag commented Nov 13, 2020

Copy link
Copy Markdown
Member Author

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

@ronag ronag force-pushed the writable-need-drain branch from e2568b0 to a0d8cb0 Compare November 13, 2020 07:46
@ronag ronag force-pushed the writable-need-drain branch from a0d8cb0 to 7a40258 Compare November 13, 2020 07:53
@ronag

ronag commented Nov 13, 2020

Copy link
Copy Markdown
Member Author

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

Comment thread test/client-stream.js Outdated
Comment thread test/client-stream.js Outdated
@ronag ronag requested a review from mcollina November 13, 2020 08:27

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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