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: correctly handle nexttick scheduling in stream reads #24022

Merged
merged 1 commit into from Jun 11, 2020

Conversation

pfrazee
Copy link
Contributor

@pfrazee pfrazee commented Jun 8, 2020

Description of Change

We discovered some cases where protocol streaming responses would stall before completing. We isolated the issue to the case where a stream's read handler scheduled a push() using nextTick(), which (according to @mafintosh, who I trust to know about this sort of thing) will get scheduled to run prior to control being returned to C++. This means the call into js' read() will return nothing, but the 'readable' event will get missed.

We were able to fix this by flagging when that occurs and automatically retrying the read.

Checklist

Release Notes

Notes: Fix an issue which would cause streaming protocol responses to stall in some cases

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 8, 2020
@mafintosh
Copy link
Contributor

Just to add a little context. nextTicks in Node er technically afterTicks as they run in the same event tick as the last thing (legacy naming).

In practice this bug manifest itself almost immediately on any stream returned that’s the last stream in a pipeline as they will have lots of these “read returns null but data is added later in the tick” events

mafintosh added a commit to beakerbrowser/beaker that referenced this pull request Jun 9, 2020
pfrazee added a commit to beakerbrowser/beaker that referenced this pull request Jun 9, 2020
@nornagon
Copy link
Member

nornagon commented Jun 9, 2020

NodeStreamLoader needs a rewrite. It's extremely subtle and full of incorrect behaviors like this 😞

Preferably, a rewrite would have the C++ side expose a much simpler "raw" API than NodeStreamLoader does, and handle translating between node's stream API and the "raw" API in JavaScript.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 9, 2020
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

Node.js streaming is so hard 😞 , thanks for fixing this!

@codebytere codebytere merged commit 81d09be into electron:master Jun 11, 2020
@release-clerk
Copy link

release-clerk bot commented Jun 11, 2020

Release Notes Persisted

Fix an issue which would cause streaming protocol responses to stall in some cases

@trop
Copy link
Contributor

trop bot commented Jun 11, 2020

I was unable to backport this PR to "8-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jun 11, 2020

I have automatically backported this PR to "10-x-y", please check out #24081

@trop
Copy link
Contributor

trop bot commented Jun 11, 2020

I have automatically backported this PR to "9-x-y", please check out #24082

@trop
Copy link
Contributor

trop bot commented Jun 15, 2020

@zcbenz has manually backported this PR to "8-x-y", please check out #24133

sentialx pushed a commit to sentialx/electron that referenced this pull request Jul 30, 2020
sentialx pushed a commit to sentialx/electron that referenced this pull request Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants