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

[Flight] add failing test to demonstrate bug in ReadableStream implementation #23140

Conversation

jplhomer
Copy link
Contributor

This PR contributes a failing test related to the following issues:

This test is fixed if we add back the "reentrancy hack" removed in this PR #22446

When a reader consumes a ReadableStream created by renderToReadableStream, it seems that calling controller.enqueue synchronously calls pull > startFlowing.

Sometimes, this leads to a stream being closed prematurely. This surfaces as an error (seen in this particular test) like:

The stream is not in a state that permits close

Other times, this leads to the stream never being closed at all. This is because request.pendingChunks ends up as a negative number due to request.pendingChunks-- being called too many times:

  console.log
    pending chunks: -5

      at flushCompletedChunks (packages/react-server/src/ReactFlightServer.js:774:11)

Since request.pendingChunks never equals 0, the request is never closed, and the reader hangs.

All of these issues are related to the competing nature of the ReadableStream enqueue/pull mechanism vs React's internal ping/retrySegment.

Note: This particular test is for Flight, but one could easily reproduce a similar failure for Fizz + renderToReadableStream.

@jplhomer jplhomer changed the title tests: add failing test to demonstrate bug in ReadableStream implementation [Flight] add failing test to demonstrate bug in ReadableStream implementation Jan 19, 2022
@sizebot
Copy link

sizebot commented Jan 19, 2022

Comparing: 790b524...b22a421

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 129.72 kB 129.72 kB = 41.58 kB 41.58 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 134.86 kB 134.86 kB = 43.11 kB 43.11 kB
facebook-www/ReactDOM-prod.classic.js = 428.15 kB 428.15 kB = 78.69 kB 78.69 kB
facebook-www/ReactDOM-prod.modern.js = 417.87 kB 417.87 kB = 77.23 kB 77.23 kB
facebook-www/ReactDOMForked-prod.classic.js = 428.15 kB 428.15 kB = 78.70 kB 78.70 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against b22a421

@jplhomer
Copy link
Contributor Author

This test is included in #23342 which resolves the issue ❤️

@jplhomer jplhomer closed this Feb 23, 2022
@jplhomer jplhomer deleted the jl-failing-test-flight-readable-stream branch February 23, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants