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

React 18: "The stream is not in a state that permits close" in renderToReadableStream #22772

Closed
jplhomer opened this issue Nov 15, 2021 · 5 comments · Fixed by #23342
Closed
Assignees
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion

Comments

@jplhomer
Copy link
Contributor

jplhomer commented Nov 15, 2021

When using renderToReadableStream for e.g. Cloudflare Workers runtimes, I'm noticing this error on the server:

TypeError: The stream is not in a state that permits close
    at ReadableStreamDefaultController.close (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/node_modules/web-streams-polyfill/src/lib/readable-stream/default-controller.ts:72:13)
    at Mc (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:3636:145)
    at Bc (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:3457:33)
    at ping (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:3128:24)
GET / 200 OK (2091.13ms)

I'm assuming this results in the following malformed chunk being sent to the stream:

<div hidden id="<div hidden id="S:1">

Which results in the client failing:

Uncaught TypeError: Cannot read properties of null (reading 'parentNode')
    at $RS (127.0.0.1/:11)
    at 127.0.0.1/:11

I put together a reproduction here, powered by Miniflare for the Workers runtime: https://github.com/jplhomer/vite-streaming-ssr-demo

Possible issues

@gaearon
Copy link
Collaborator

gaearon commented Nov 18, 2021

Have you looked at what chain of assumptions leads to this? It seems like it would be good to understand what different state machines are involved and why something's happening in CF that doesn't happen in the browser.

@jplhomer jplhomer changed the title React 18: "The stream is not in a state that permits close" in pipeToReadableStream React 18: "The stream is not in a state that permits close" in renderToReadableStream Dec 1, 2021
@jplhomer
Copy link
Contributor Author

jplhomer commented Dec 1, 2021

Have you looked at what chain of assumptions leads to this?

@gaearon after lots of digging, here's what we've discovered:

The difference between Cloudflare/Oxygen Workers runtimes and the browser comes down to how the stream is consumed after being passed to Response.

In the browser and related fizz tests, this is how the stream (via response.body) is consumed:

await response.blob();

This presumably results in a single pull.

In the worker runtime, response.body is read through an async iterator:

for await (const chunk of response.body) {
  // write chunk from server
}

Importantly, this results in pull getting called multiple times as more data is enqueued and as the reader sees fit. This is an assumption on my part, so I could be wrong here.

When combined with Suspense boundaries, pingTask callbacks trigger flushCompletedQueues. At the same time, repeated pull requests from the stream reader also call flushCompletedQueues.

This results in the following error on the server:

Error: Aborted, errored or already flushed boundaries should not be flushed again. This is a bug in React.
    at flushSubtree (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:12538:15)
    at flushSegment (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:12545:14)
    at flushSegmentContainer (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:12583:5)
    at flushPartiallyCompletedSegment (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:12622:7)
    at flushCompletedBoundary (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:12591:7)
    at flushCompletedQueues (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:12659:14)
    at performWork (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:12497:9)
    at /Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:11752:16
    at scheduleWork (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:8042:5)
    at pingTask (/Users/joshlarson/src/github.com/jplhomer/vite-streaming-ssr-demo/dist/worker/worker.js:11751:7)

You can also reproduce this directly in the browser by attempting to read the stream with response.body.getReader(). Here is a new fixture with a reproduction of the issue, including a couple test cases that show the different errors you'll see based on the number of Suspense boundaries you have:

https://github.com/jplhomer/react/blob/jl-fizz-ssr-streaming-browser/fixtures/fizz-ssr-browser-streaming/index.html

If this seems like a legitimate bug to you, a couple ideas I had:

  • Should there be some sort of check like if (isAlreadyFlushingQueues) return in flushCompletedQueues?
  • Should we splice completed boundaries earlier to account for concurrency so they are removed from the array and not re-flushed?

Here are some other resources and scratch notes: jplhomer/vite-streaming-ssr-demo#1 (comment)

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Feb 23, 2022

We'll fix this reentrancy bug somehow. However, right now it's a bit lower priority because CloudFlare technically don't support ReadableStreams and so you have to go through the polyfill and any subtle differences might be due to the polyfill. To really know if we fixed this issue we should apply this to native CloudFlare ReadableStreams... which I'm told is coming. So that's how we know if it actually worked. Until then, the support for CloudFlare workers is... partial.

On a second thought this should probably fix it. I think I finally understand how it's supposed to work now. #23342

sebmarkbage added a commit to sebmarkbage/react that referenced this issue Feb 23, 2022
I removed the equivalency of this in facebook#22446. However, I didn't fully
understand the intended semantics of the spec but I understand this better.

I believe this fixes facebook#22772.

This includes the test from facebook#22889 but should ideally have one for Fizz.
sebmarkbage added a commit to sebmarkbage/react that referenced this issue Feb 23, 2022
I removed the equivalency of this in facebook#22446. However, I didn't fully
understand the intended semantics of the spec but I understand this better
now.

The spec is not actually recursive. It won't call pull again inside of a
pull. It might not call it inside startWork neither which the original
issue avoided. However, it will call pull if you enqueue to the controller
without filling up the desired size outside any call.

We could avoid that by returning a Promise from pull that we wait to
resolve until we've performed all our pending tasks. That would be the
more idiomatic solution. That's a bit more involved but since we know
understand it, we can readd the reentrancy hack since we have an easy place
to detect it. If anything, it should probably throw or log here otherwise.

I believe this fixes facebook#22772.

This includes the test from facebook#22889 but should ideally have one for Fizz.
sebmarkbage added a commit that referenced this issue Feb 23, 2022
* tests: add failing test to demonstrate bug in ReadableStream implementation

* Re-add reentrancy avoidance

I removed the equivalency of this in #22446. However, I didn't fully
understand the intended semantics of the spec but I understand this better
now.

The spec is not actually recursive. It won't call pull again inside of a
pull. It might not call it inside startWork neither which the original
issue avoided. However, it will call pull if you enqueue to the controller
without filling up the desired size outside any call.

We could avoid that by returning a Promise from pull that we wait to
resolve until we've performed all our pending tasks. That would be the
more idiomatic solution. That's a bit more involved but since we know
understand it, we can readd the reentrancy hack since we have an easy place
to detect it. If anything, it should probably throw or log here otherwise.

I believe this fixes #22772.

This includes the test from #22889 but should ideally have one for Fizz.

Co-authored-by: Josh Larson <josh.larson@shopify.com>
@sebmarkbage
Copy link
Collaborator

@jplhomer Please take another look and see if this fixes all the issues you've noticed.

@jplhomer
Copy link
Contributor Author

@sebmarkbage works like a charm! Thanks for taking the time to look at it and get it fixed 🎉

zhengjitf pushed a commit to zhengjitf/react that referenced this issue Apr 15, 2022
* tests: add failing test to demonstrate bug in ReadableStream implementation

* Re-add reentrancy avoidance

I removed the equivalency of this in facebook#22446. However, I didn't fully
understand the intended semantics of the spec but I understand this better
now.

The spec is not actually recursive. It won't call pull again inside of a
pull. It might not call it inside startWork neither which the original
issue avoided. However, it will call pull if you enqueue to the controller
without filling up the desired size outside any call.

We could avoid that by returning a Promise from pull that we wait to
resolve until we've performed all our pending tasks. That would be the
more idiomatic solution. That's a bit more involved but since we know
understand it, we can readd the reentrancy hack since we have an easy place
to detect it. If anything, it should probably throw or log here otherwise.

I believe this fixes facebook#22772.

This includes the test from facebook#22889 but should ideally have one for Fizz.

Co-authored-by: Josh Larson <josh.larson@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants