Skip to content

Commit

Permalink
[Fizz] Prevent uncloned large precomputed chunks without relying on r…
Browse files Browse the repository at this point in the history
…ender-time assertions (#28568)

A while back we implemented a heuristic that if a chunk was large it was
assumed to be produced by the render and thus was safe to stream which
results in transferring the underlying object memory. Later we ran into
an issue where a precomputed chunk grew large enough to trigger this
hueristic and it started causing renders to fail because once a second
render had occurred the precomputed chunk would not have an underlying
buffer of bytes to send and these bytes would be omitted from the
stream. We implemented a technique to detect large precomputed chunks
and we enforced that these always be cloned before writing.
Unfortunately our test coverage was not perfect and there has been for a
very long time now a usage pattern where if you complete a boundary in
one flush and then complete a boundary that has stylehsheet dependencies
in another flush you can get a large precomputed chunk that was not
being cloned to be sent twice causing streaming errors.

I've thought about why we even went with this solution in the first
place and I think it was a mistake. It relies on a dev only check to
catch paired with potentially version specific order of operations on
the streaming side. This is too unreliable. Additionally the low limit
of view size for Edge is not used in Node.js but there is not real
justification for this.

In this change I updated the view size for edge streaming to match Node
at 2048 bytes which is still relatively small and we have no data one
way or another to preference 512 over this. Then I updated the assertion
logic to error anytime a precomputed chunk exceeds the size. This
eliminates the need to clone these chunks by just making sure our view
size is always larger than the largest precomputed chunk we can possibly
write. I'm generally in favor of this for a few reasons.

First, we'll always know during testing whether we've violated the limit
as long as we exercise each stream config because the precomputed chunks
are created in module scope. Second, we can always split up large chunks
so making sure the precomptued chunk is smaller than whatever view size
we actually desire is relatively trivial.

DiffTrain build for [b09e102](b09e102)
  • Loading branch information
gnoff committed Mar 16, 2024
1 parent cb7bf99 commit d06d633
Show file tree
Hide file tree
Showing 6 changed files with 8 additions and 26 deletions.
2 changes: 1 addition & 1 deletion compiled/facebook-www/REVISION
Original file line number Diff line number Diff line change
@@ -1 +1 @@
a870b2d5494351d75b68c3d9baf03a52fd40a8ef
b09e102ff1e2aaaf5eb6585b04609ac7ff54a5c8
2 changes: 1 addition & 1 deletion compiled/facebook-www/React-prod.modern.js
Original file line number Diff line number Diff line change
Expand Up @@ -625,4 +625,4 @@ exports.useSyncExternalStore = function (
exports.useTransition = function () {
return ReactCurrentDispatcher.current.useTransition();
};
exports.version = "18.3.0-www-modern-26dfe226";
exports.version = "18.3.0-www-modern-94e2c94e";
10 changes: 2 additions & 8 deletions compiled/facebook-www/ReactDOMServer-dev.classic.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ if (__DEV__) {
var React = require("react");
var ReactDOM = require("react-dom");

var ReactVersion = "18.3.0-www-classic-3a1cc8c2";
var ReactVersion = "18.3.0-www-classic-c69e4b0a";

// This refers to a WWW module.
var warningWWW = require("warning");
Expand Down Expand Up @@ -548,9 +548,6 @@ if (__DEV__) {
function stringToPrecomputedChunk(content) {
return content;
}
function clonePrecomputedChunk(chunk) {
return chunk;
}
function closeWithError(destination, error) {
// $FlowFixMe[incompatible-call]: This is an Error object or the destination accepts other types.
destination.destroy(error);
Expand Down Expand Up @@ -6422,10 +6419,7 @@ if (__DEV__) {
) {
resumableState.instructions |=
SentStyleInsertionFunction | SentCompleteBoundaryFunction;
writeChunk(
destination,
clonePrecomputedChunk(completeBoundaryWithStylesScript1FullBoth)
);
writeChunk(destination, completeBoundaryWithStylesScript1FullBoth);
} else if (
(resumableState.instructions & SentStyleInsertionFunction) ===
NothingSent
Expand Down
10 changes: 2 additions & 8 deletions compiled/facebook-www/ReactDOMServer-dev.modern.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ if (__DEV__) {
var React = require("react");
var ReactDOM = require("react-dom");

var ReactVersion = "18.3.0-www-modern-7205a424";
var ReactVersion = "18.3.0-www-modern-ac7e39a5";

// This refers to a WWW module.
var warningWWW = require("warning");
Expand Down Expand Up @@ -548,9 +548,6 @@ if (__DEV__) {
function stringToPrecomputedChunk(content) {
return content;
}
function clonePrecomputedChunk(chunk) {
return chunk;
}
function closeWithError(destination, error) {
// $FlowFixMe[incompatible-call]: This is an Error object or the destination accepts other types.
destination.destroy(error);
Expand Down Expand Up @@ -6422,10 +6419,7 @@ if (__DEV__) {
) {
resumableState.instructions |=
SentStyleInsertionFunction | SentCompleteBoundaryFunction;
writeChunk(
destination,
clonePrecomputedChunk(completeBoundaryWithStylesScript1FullBoth)
);
writeChunk(destination, completeBoundaryWithStylesScript1FullBoth);
} else if (
(resumableState.instructions & SentStyleInsertionFunction) ===
NothingSent
Expand Down
8 changes: 1 addition & 7 deletions compiled/facebook-www/ReactDOMServerStreaming-dev.modern.js
Original file line number Diff line number Diff line change
Expand Up @@ -544,9 +544,6 @@ if (__DEV__) {
function stringToPrecomputedChunk(content) {
return content;
}
function clonePrecomputedChunk(chunk) {
return chunk;
}
function closeWithError(destination, error) {
destination.done = true;
destination.fatal = true;
Expand Down Expand Up @@ -6418,10 +6415,7 @@ if (__DEV__) {
) {
resumableState.instructions |=
SentStyleInsertionFunction | SentCompleteBoundaryFunction;
writeChunk(
destination,
clonePrecomputedChunk(completeBoundaryWithStylesScript1FullBoth)
);
writeChunk(destination, completeBoundaryWithStylesScript1FullBoth);
} else if (
(resumableState.instructions & SentStyleInsertionFunction) ===
NothingSent
Expand Down
2 changes: 1 addition & 1 deletion compiled/facebook-www/__test_utils__/ReactAllWarnings.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit d06d633

Please sign in to comment.