Fix streaming SSR in react-dom/server.browser
#22889
Conversation
RE: "The stream is not in a state that permits close" in `renderToReadableStream` facebook#22772
|
Hi @jplhomer! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Comparing: f2a59df...1eb23c3 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
|
FYI: this PR looks super similar, so I decided to test my fixture against it: #22726 I'm still getting the same error, so unfortunately that wasn't the fix, either. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
This seems similar to the hack I removed in #22446 I was able to work around that specific issue in another way. Ideally we should try working around it here too. It's a bit annoying because we'd like to not have these hacks - and especially in the core implementation rather than a specific runtime. It's extra annoying because the browser implementation doesn't follow the spec here and so it's not covered by the non-browser test. Is this also a case where Workers aren't following the spec? |
|
@sebmarkbage it looks like the workaround in the PR you linked was to prevent calling Could you share what specific thing the browser implementation isn't following in the spec here? Happy to dig in further and maybe add a workaround in the So far, we've observed this behavior in:
|
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.
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.
* 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 <[email protected]>
|
Fixed by #23342 |
* 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 <[email protected]>
Summary
In #22772, I document an issue we've discovered while using
react-dom/server.browserin both a web browser and a Workers runtime (like Cloudflare Workers or Shopify Oxygen).The root of the problem appears to be related to the fact that the reader of a
ReadableStreamcan callpullwhenever it pleases, which triggersflushCompletedQueues. This conflicts with Suspense boundaries which enqueue additional calls toflushCompletedQueuesviapingTask.This problem has not yet surfaced in existing fixtures (
fizz-ssr-browser) becauseawait response.blob()is called, which results inpullbeing called exactly once more after the stream is locked.This PR adds a new status to
RequestcalledFLUSHINGwhich gets set at the top offlushCompletedQueuesand returned back toOPENin thefinallyblock.I have no idea if this is the correct solution, but it seems to solve the problem in the test fixture I've provided in this PR.
How did you test this change?
To replicate this problem, comment out the changes made to
flushCompletedQueuesin this PR and run:This should open a page in your web browser which results in the following:
You can get a slightly different error if you add an additional Suspense boundary! Uncomment the second boundary in the
fixtures/fizz-ssr-browser-streaming/index.htmlfile and reload the page:To resolve this issue, you can uncomment the changes made in this PR to
flushCompletedQueuesand run this again:Now you should see the streaming resolve fine without competing calls to
flushCompletedQueues(for a single and for multiple Suspense boundaries):