[labs/ssr] fix: RenderResultReadable skipping async work#4389
Merged
AndrewJakubowicz merged 5 commits intomainfrom Nov 11, 2023
Merged
[labs/ssr] fix: RenderResultReadable skipping async work#4389AndrewJakubowicz merged 5 commits intomainfrom
AndrewJakubowicz merged 5 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: f69076e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultsthis-change
render
update
update-reflect
this-change, tip-of-tree, previous-release
render
update
nop-update
this-change, tip-of-tree, previous-release
render
update
this-change, tip-of-tree, previous-release
render
update
update-reflect
|
Contributor
|
The size of lit-html.js and lit-core.min.js are as expected. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Fix a race condition bug with RenderResultReadable.
What is the bug?
The
RenderResultReadableclass lets us efficiently stream synchronous or async iterables. It's architecture is designed to be as optimal as possible, so synchronous streams go fast and have minimal overhead from the async handling.Our
RenderResultReadable.prototype._readimplementation is async, allowing us toawaitvalues in the stream.From Node.js documentation: "
_read()will be called again after each call tothis.push(dataChunk)once the stream is ready to accept more data."This is also the behavior back to Node.js v10.
What happens for the test case below (without the fix) when we are reading from the stream:
_read(). We synchronouslypush('a')._readencounters the promise and awaits the promise._read(), and we synchronouslypush('c'), then we loop and synchronously callpush(null)terminating the stream.Thus the promise is skipped.
Fix
Once
_read()is called, it will not be called again untilreadable.push()has been called. This makes it safe for us to noop a_read()call while a Promise is waiting. Then once that promise resolves, it will result inpush()being called with the resolved data and we're back in a consistent state.Test plan
I added two tests that reproduce without the
_waitingflag.