[Flight] Don't try to close debug channel twice #34340
Conversation
When the debug channel was already closed, we must not try to close it
again when the Response gets garbage collected.
Test plan:
1. reduce the Flight fixture App component to a minimum
- remove everything from `<body>`
- delete the `console.log` statement
2. open the app in Firefox (seems to have a more aggressive GC strategy)
3. wait a few seconds
On `main`, you will see the following error in the browser console:
```
TypeError: Can not close stream after closing or error
```
With this change, the error is gone.
It's a bit concerning that step 1 is needed to reproduce the issue.
Either GC is behaving differently with the unmodified App, or we may
hold on to the Response under certain conditions, potentially creating
a memory leak. This needs further investigation.
github-actions
bot
added
the
React Core Team
|
Comparing: 3fe51c9...a34a4c1 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| closeDebugChannel(debugChannel); | ||
| response._debugChannel = undefined; | ||
| // Make sure the debug channel is not closed a second time when the | ||
| // Response gets GC:ed. |
There was a problem hiding this comment.
"GC:ed" is how we spell it in other places as well. On a related note: https://chatgpt.com/share/68b1a173-dfe8-8000-9114-bc44a40feaa8 😁
|
I believe that Fiber currently favors retaining owners from the original owner or stack vs updating to a new info from a new element. I think I originally meant this to optimize owner stacks to avoid re-reifying new stacks every time something rerenders. However, this means that the debug information from those can retain the whole Response and keep it alive. Potentially forever as long as one instance is still mounted. Another thing is that any Error that is not reified can hold to the stack frames as part of that stack until something access the Unless you keep adding one instance from every rerender it shouldn't be indefinitely though. Eventually you'll only have so many places that retain something that it eventually starts cleaning up but there can be many different Responses alive for a long time. Potentially the first one forever. Two things we could do is 1) always override the debug stack/owner with the latest value on the Fiber 2) reify the |
When the debug channel was already closed, we must not try to close it
again when the Response gets garbage collected.
**Test plan:**
1. reduce the Flight fixture `App` component to a minimum [^1]
- remove everything from `<body>`
- delete the `console.log` statement
2. open the app in Firefox (seems to have a more aggressive GC strategy)
3. wait a few seconds
On `main`, you will see the following error in the browser console:
```
TypeError: Can not close stream after closing or error
```
With this change, the error is gone.
[^1]: It's a bit concerning that step 1 is needed to reproduce the
issue. Either GC is behaving differently with the unmodified App, or we
may hold on to the Response under certain conditions, potentially
creating a memory leak. This needs further investigation.
DiffTrain build for [aad7c66](facebook@aad7c66)
When the debug channel was already closed, we must not try to close it
again when the Response gets garbage collected.
**Test plan:**
1. reduce the Flight fixture `App` component to a minimum [^1]
- remove everything from `<body>`
- delete the `console.log` statement
2. open the app in Firefox (seems to have a more aggressive GC strategy)
3. wait a few seconds
On `main`, you will see the following error in the browser console:
```
TypeError: Can not close stream after closing or error
```
With this change, the error is gone.
[^1]: It's a bit concerning that step 1 is needed to reproduce the
issue. Either GC is behaving differently with the unmodified App, or we
may hold on to the Response under certain conditions, potentially
creating a memory leak. This needs further investigation.
DiffTrain build for [aad7c66](facebook@aad7c66)
When the debug channel was already closed, we must not try to close it
again when the Response gets garbage collected.
**Test plan:**
1. reduce the Flight fixture `App` component to a minimum [^1]
- remove everything from `<body>`
- delete the `console.log` statement
2. open the app in Firefox (seems to have a more aggressive GC strategy)
3. wait a few seconds
On `main`, you will see the following error in the browser console:
```
TypeError: Can not close stream after closing or error
```
With this change, the error is gone.
[^1]: It's a bit concerning that step 1 is needed to reproduce the
issue. Either GC is behaving differently with the unmodified App, or we
may hold on to the Response under certain conditions, potentially
creating a memory leak. This needs further investigation.
4 participants
When the debug channel was already closed, we must not try to close it again when the Response gets garbage collected.
Test plan:
Appcomponent to a minimum 1<body>console.logstatementOn
main, you will see the following error in the browser console:With this change, the error is gone.
Footnotes
It's a bit concerning that step 1 is needed to reproduce the issue. Either GC is behaving differently with the unmodified App, or we may hold on to the Response under certain conditions, potentially creating a memory leak. This needs further investigation. ↩