[Fizz/Flight] Remove reentrancy hack #22446
Merged
Conversation
This file contains 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
facebook-github-bot
added
the
React Core Team
|
Comparing: 6638815...2a6f55b 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
|
This is already an explicit call in Fizz. This moves flowing to be explicit. That way we can avoid calling it in start() for web streams and therefore avoid the reentrant call.
sebmarkbage
force-pushed
the
rmrentrancy
branch
from
8f0fd1a
to
ff525b6
Compare
This test doesn't actually error due to the streams polyfill not behaving like Chrome but rather according to spec.
Not that we need this but just in case there are differences that are fixed.
sebmarkbage
force-pushed
the
rmrentrancy
branch
from
ff525b6
to
2a6f55b
Compare
sebmarkbage
changed the title
salazarm
approved these changes
Sep 28, 2021
|
|
||
| expect(reportedErrors).toEqual([]); | ||
|
|
||
| const theError = new Error('Game over'); |
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Oct 12, 2021
Summary: This sync includes the following changes: - **[579c008a7](facebook/react@579c008a7 )**: [Fizz/Flight] pipeToNodeWritable(..., writable).startWriting() -> renderToPipeableStream(...).pipe(writable) ([#22450](facebook/react#22450)) //<Sebastian Markbåge>// - **[f2c381131](facebook/react@f2c381131 )**: fix: useSyncExternalStoreExtra ([#22500](facebook/react#22500)) //<Daishi Kato>// - **[0ecbbe142](facebook/react@0ecbbe142 )**: Sync hydrate discrete events in capture phase and dont replay discrete events ([#22448](facebook/react#22448)) //<salazarm>// - **[a724a3b57](facebook/react@a724a3b57 )**: [RFC] Codemod invariant -> throw new Error ([#22435](facebook/react#22435)) //<Andrew Clark>// - **[201af81b0](facebook/react@201af81b0 )**: Release pooled cache reference in complete/unwind ([#22464](facebook/react#22464)) //<Joseph Savona>// - **[033efe731](facebook/react@033efe731 )**: Call get snapshot in useSyncExternalStore server shim ([#22453](facebook/react#22453)) //<salazarm>// - **[7843b142a](facebook/react@7843b142a )**: [Fizz/Flight] Pass in Destination lazily to startFlowing instead of in createRequest ([#22449](facebook/react#22449)) //<Sebastian Markbåge>// - **[d9fb383d6](facebook/react@d9fb383d6 )**: Extract queueing logic into shared functions ([#22452](facebook/react#22452)) //<Andrew Clark>// - **[9175f4d15](facebook/react@9175f4d15 )**: Scheduling Profiler: Show Suspense resource .displayName ([#22451](facebook/react#22451)) //<Brian Vaughn>// - **[eba248c39](facebook/react@eba248c39 )**: [Fizz/Flight] Remove reentrancy hack ([#22446](facebook/react#22446)) //<Sebastian Markbåge>// - **[66388150e](facebook/react@66388150e )**: Remove usereducer eager bailout ([#22445](facebook/react#22445)) //<Joseph Savona>// - **[d3e086932](facebook/react@d3e086932 )**: Make root.unmount() synchronous ([#22444](facebook/react#22444)) //<Andrew Clark>// - **[2cc6d79c9](facebook/react@2cc6d79c9 )**: Rename onReadyToStream to onCompleteShell ([#22443](facebook/react#22443)) //<Sebastian Markbåge>// - **[c88fb49d3](facebook/react@c88fb49d3 )**: Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) ([#22064](facebook/react#22064)) //<Justin Grant>// - **[05726d72c](facebook/react@05726d72c )**: [Fix] Errors should not "unsuspend" a transition ([#22423](facebook/react#22423)) //<Andrew Clark>// - **[3746eaf98](facebook/react@3746eaf98 )**: Packages/React/src/ReactLazy ---> changing -1 to unintialized ([#22421](facebook/react#22421)) //<BIKI DAS>// - **[04ccc01d9](facebook/react@04ccc01d9 )**: Hydration errors should force a client render ([#22416](facebook/react#22416)) //<Andrew Clark>// - **[029fdcebb](facebook/react@029fdcebb )**: root.hydrate -> root.isDehydrated ([#22420](facebook/react#22420)) //<Andrew Clark>// - **[af87f5a83](facebook/react@af87f5a83 )**: Scheduling Profiler marks should include thrown Errors ([#22417](facebook/react#22417)) //<Brian Vaughn>// - **[d47339ea3](facebook/react@d47339ea3 )**: [Fizz] Remove assignID mechanism ([#22410](facebook/react#22410)) //<Sebastian Markbåge>// - **[3a50d9557](facebook/react@3a50d9557 )**: Never attach ping listeners in legacy Suspense ([#22407](facebook/react#22407)) //<Andrew Clark>// - **[82c8fa90b](facebook/react@82c8fa90b )**: Add back useMutableSource temporarily ([#22396](facebook/react#22396)) //<Andrew Clark>// - **[5b57bc6e3](facebook/react@5b57bc6e3 )**: [Draft] don't patch console during first render ([#22308](facebook/react#22308)) //<Luna Ruan>// - **[cf07c3df1](facebook/react@cf07c3df1 )**: Delete all but one `build2` reference ([#22391](facebook/react#22391)) //<Andrew Clark>// - **[bb0d06935](facebook/react@bb0d06935 )**: [build2 -> build] Local scripts //<Andrew Clark>// - **[0c81d347b](facebook/react@0c81d347b )**: Write artifacts to `build` instead of `build2` //<Andrew Clark>// - **[4da03c9fb](facebook/react@4da03c9fb )**: useSyncExternalStore React Native version ([#22367](facebook/react#22367)) //<salazarm>// - **[48d475c9e](facebook/react@48d475c9e )**: correct typos ([#22294](facebook/react#22294)) //<Bowen>// - **[cb6c619c0](facebook/react@cb6c619c0 )**: Remove Fiber fields that were used for hydrating useMutableSource ([#22368](facebook/react#22368)) //<Sebastian Silbermann>// - **[64e70f82e](facebook/react@64e70f82e )**: [Fizz] add avoidThisFallback support ([#22318](facebook/react#22318)) //<salazarm>// - **[3ee7a004e](facebook/react@3ee7a004e )**: devtools: Display actual ReactDOM API name in root type ([#22363](facebook/react#22363)) //<Sebastian Silbermann>// - **[79b8fc667](facebook/react@79b8fc667 )**: Implement getServerSnapshot in userspace shim ([#22359](facebook/react#22359)) //<Andrew Clark>// - **[86b3e2461](facebook/react@86b3e2461 )**: Implement useSyncExternalStore on server ([#22347](facebook/react#22347)) //<Andrew Clark>// - **[8209de269](facebook/react@8209de269 )**: Delete useMutableSource implementation ([#22292](facebook/react#22292)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions e8feb11...afcb9cd jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D31541359 fbshipit-source-id: c35941bc303fdf55cb061e9996200dc868a6f2af
|
Hey @sebmarkbage it looks like Here's a new failing test: #23140 After digging, this behavior appears to match the spec. The reason the test you provided in this PR doesn't fail is because we never check for It also seems like Per the spec, would it make sense to experiment with returning a Promise from
|
|
Is it the case that pull isn't called again if enqueueing happens inside the first pull? Only when it's first called outside of another pull. That kind of makes sense. Because otherwise returning a Promise wouldn't help since the recursion would happen before it's even returned. |
|
Hmm yeah, I tried returning a Promise from |
4 tasks
sebmarkbage
added a commit
to sebmarkbage/react
that referenced
this pull request
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 pull request
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 pull request
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 <[email protected]>
zhengjitf
pushed a commit
to zhengjitf/react
that referenced
this pull request
Apr 15, 2022
* Remove reentrant check from Fizz/Flight * Make startFlowing explicit in Flight This is already an explicit call in Fizz. This moves flowing to be explicit. That way we can avoid calling it in start() for web streams and therefore avoid the reentrant call. * Add regression test This test doesn't actually error due to the streams polyfill not behaving like Chrome but rather according to spec. * Update the Web Streams polyfill Not that we need this but just in case there are differences that are fixed.
zhengjitf
pushed a commit
to zhengjitf/react
that referenced
this pull request
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 <[email protected]>
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.
This code is a bit annoying because it's only reproducable in real browsers and not the web streams polyfill we use for our jest tests. It's covered by our fixtures though. The annoying part is that the polyfill is correct according to the spec.
In real browsers, if you call
controller.enqueue(...)inside thestart(...)method, it will synchronously callpull(...)which will call start flowing again and will callflushCompletedChunks.However, this should ideally never happen because
scheduleWork()shouldn't be sync inside thestart(...)method anyway.In Fizz I already moved to starting the flow after the stream has connected (locked) rather than immediately. This ensures that if it's delayed to be emitted, we can give it a better prioritized stream. https://github.com/facebook/react/blob/main/packages/react-dom/src/server/ReactDOMFizzServerBrowser.js#L68
This also ensures that we don't call
enqueueinstart().I'm not sure 100% this is still safe if enqueue. We probably could check if we're already flowing inside
startFlowingand if so bail early. But let's try this for now and see if we discover more cases.