Uh oh!
There was an error while loading. Please reload this page.
Fix unbounded debugInfo growth in Flight transferReferencedDebugInfo - #37358
Fix unbounded debugInfo growth in Flight transferReferencedDebugInfo#37358sundeep8967 wants to merge 2 commits into
Conversation
Hi @sundeep8967! 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 cla@meta.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
The Flight Client copies the debug info of a referenced chunk into the chunk that references it, so that the receiving chunk records what blocked it. It copies the entries once per reference, and a referenced chunk already carries the entries that it received itself. A response that deduplicates the same object across a chain of rows therefore multiplies the entries at every step. In development the array eventually grows past what the engine can allocate for it, and the client throws `RangeError: Invalid array length`. The receiving chunk now takes each entry only once. The entries are copied by reference and never cloned on this path, so a comparison by identity is exact. The array becomes bounded by the number of distinct entries in the response rather than by a chosen limit. The bookkeeping costs one `Set` per chunk that receives debug info, in development only. The set holds a reference to each entry rather than a copy, so the entries stay shared and nothing about the debug info is duplicated. A chunk receives entries only while it is blocked, so the fix releases the set as soon as the chunk initializes. Debug info still accumulates transitively, which this change does not alter. This change also adds the `!reference.isDebug` guards that react#37358 proposes for the element props branch and the default branch of `fulfillReference`. react#35795 introduced the rule that a reference resolved during debug info resolution does not transfer, and it left those two branches behind. The guards make that rule hold at every branch. However, those branches reference debug chunks that carry no entries, so the guards change no observed behaviour, and they do not fix the growth in react#37343, which comes from references in model chunks. react#37343 also reports the call in `getOutlinedModel` as unguarded, which it is not, because react#35795 already skips it there. The rest of react#37358 deduplicates the entries, which is the right direction, but it scans the receiving array for every candidate, which is quadratic in the size of the debug info. react#37359 caps the array at a constant instead, which stops the crash but keeps copying the duplicates and drops debug info once a response passes the cap. Alternatives Considered - Tracking the referenced chunks rather than the entries would be cheaper, because it would need one map entry per referenced chunk. It would not be enough, because a chunk can reach the same entry through two paths. A chunk can hold a client reference directly and also reference a chunk that already received the debug info of that client reference. - Recording the last receiving chunk on each entry would be exact while a chunk parses its model, where the transfers into it are consecutive. It would break once transfers into different chunks interleave, and that is the path the reported crash takes. - Turning `_debugInfo` itself into a `Set` is not possible, because the reconciler, Fizz, the Flight Server and DevTools read it by index and depend on its order. Fixesreact#37343Closesreact#37358Closesreact#37359 Co-authored-by: sundeep8967 <71071718+sundeep8967@users.noreply.github.com>
The Flight Client copies the debug info of a referenced chunk into the chunk that references it, so that the receiving chunk records what blocked it. It copies the entries once per reference, and a referenced chunk already carries the entries that it received itself. A response that deduplicates the same object across a chain of rows therefore multiplies the entries at every step. In development the array eventually grows past what the engine can allocate for it, and the client throws `RangeError: Invalid array length`. The receiving chunk now takes each entry only once. The entries are copied by reference and never cloned on this path, so a comparison by identity is exact. The array becomes bounded by the number of distinct entries in the response rather than by a chosen limit. The bookkeeping costs one `Set` per chunk that receives debug info, in development only. The set holds a reference to each entry rather than a copy, so the entries stay shared and nothing about the debug info is duplicated. A chunk receives entries only while it is blocked, so the fix releases the set as soon as the chunk initializes. Debug info still accumulates transitively, which this change does not alter. This change also adds the `!reference.isDebug` guards that #37358 proposes for the element props branch and the default branch of `fulfillReference`. #35795 introduced the rule that a reference resolved during debug info resolution does not transfer, and it left those two branches behind. The guards make that rule hold at every branch. However, those branches reference debug chunks that carry no entries, so the guards change no observed behaviour, and they do not fix the growth in #37343, which comes from references in model chunks. #37343 also reports the call in `getOutlinedModel` as unguarded, which it is not, because #35795 already skips it there. The rest of #37358 deduplicates the entries, which is the right direction, but it scans the receiving array for every candidate, which is quadratic in the size of the debug info. #37359 caps the array at a constant instead, which stops the crash but keeps copying the duplicates and drops debug info once a response passes the cap. **Alternatives Considered** - Tracking the referenced chunks rather than the entries would be cheaper, because it would need one map entry per referenced chunk. It would not be enough, because a chunk can reach the same entry through two paths. A chunk can hold a client reference directly and also reference a chunk that already received the debug info of that client reference. - Recording the last receiving chunk on each entry would be exact while a chunk parses its model, where the transfers into it are consecutive. It would break once transfers into different chunks interleave, and that is the path the reported crash takes. - Turning `_debugInfo` itself into a `Set` is not possible, because the reconciler, Fizz, the Flight Server and DevTools read it by index and depend on its order. Fixes#37343Closes#37358Closes#37359 Co-authored-by: sundeep8967 <71071718+sundeep8967@users.noreply.github.com>
The Flight Client copies the debug info of a referenced chunk into the chunk that references it, so that the receiving chunk records what blocked it. It copies the entries once per reference, and a referenced chunk already carries the entries that it received itself. A response that deduplicates the same object across a chain of rows therefore multiplies the entries at every step. In development the array eventually grows past what the engine can allocate for it, and the client throws `RangeError: Invalid array length`. The receiving chunk now takes each entry only once. The entries are copied by reference and never cloned on this path, so a comparison by identity is exact. The array becomes bounded by the number of distinct entries in the response rather than by a chosen limit. The bookkeeping costs one `Set` per chunk that receives debug info, in development only. The set holds a reference to each entry rather than a copy, so the entries stay shared and nothing about the debug info is duplicated. A chunk receives entries only while it is blocked, so the fix releases the set as soon as the chunk initializes. Debug info still accumulates transitively, which this change does not alter. This change also adds the `!reference.isDebug` guards that #37358 proposes for the element props branch and the default branch of `fulfillReference`. #35795 introduced the rule that a reference resolved during debug info resolution does not transfer, and it left those two branches behind. The guards make that rule hold at every branch. However, those branches reference debug chunks that carry no entries, so the guards change no observed behaviour, and they do not fix the growth in #37343, which comes from references in model chunks. #37343 also reports the call in `getOutlinedModel` as unguarded, which it is not, because #35795 already skips it there. The rest of #37358 deduplicates the entries, which is the right direction, but it scans the receiving array for every candidate, which is quadratic in the size of the debug info. #37359 caps the array at a constant instead, which stops the crash but keeps copying the duplicates and drops debug info once a response passes the cap. **Alternatives Considered** - Tracking the referenced chunks rather than the entries would be cheaper, because it would need one map entry per referenced chunk. It would not be enough, because a chunk can reach the same entry through two paths. A chunk can hold a client reference directly and also reference a chunk that already received the debug info of that client reference. - Recording the last receiving chunk on each entry would be exact while a chunk parses its model, where the transfers into it are consecutive. It would break once transfers into different chunks interleave, and that is the path the reported crash takes. - Turning `_debugInfo` itself into a `Set` is not possible, because the reconciler, Fizz, the Flight Server and DevTools read it by index and depend on its order. Fixes#37343Closes#37358Closes#37359 Co-authored-by: sundeep8967 <71071718+sundeep8967@users.noreply.github.com> DiffTrain build for [8f00437](8f00437)
Closes#37343
Problem
In React Flight client DEV mode,
transferReferencedDebugInfocopiesname == nullentries from a referenced chunk's_debugInfointo the parent chunk's_debugInfo. In large, deeply nested, or heavily deduplicated RSC model graphs:fulfillReference, the element props branch (case '3') anddefaultbranch calledtransferReferencedDebugInfowithout checking!reference.isDebug(unlike the general reference branch).transferReferencedDebugInfounconditionally pushed entries without checking if the parent_debugInfoalready contained that entry reference.RangeError: Invalid array length.Solution
!reference.isDebugguards to the elementcase '3'anddefaultbranches infulfillReferenceto ensure debug chunks do not recursively re-transfer debug info during resolution.transferReferencedDebugInfobefore pushing toparentDebugInfo.