Context leaks to later renders when render stream destroyed early

[overlookmotel]

Do you want to request a feature or report a bug?

Bug.

What is the current behavior?

If a stream created with .renderToNodeStream() is destroyed before the render completes, memory can be retained unnecessarily by Contexts which were active at the time.

It's not a memory leak per se as memory use is not unbounded. There's a limit on number of threads in use and context slots are reused, so memory use can't increase endlessly.

Would only make a noticeable impact on an application which:

• renders using .renderToNodeStream() or .renderToStaticMarkup()
• stores large objects in Context (e.g. large data sets from fetch requests)
• experiences a spike in traffic which then recedes
• destroys a lot of streams prematurely (or has errors thrown in rendering which causes streams to be destroyed)

In the above case, when the spike hits, many threads are created for concurrent renderers. Each thread writes data into a slot of the Contexts it uses. When the threads are destroyed before the renders complete, the Context slots are not cleared, and so left filled with data which is no longer useful, but cannot be freed by GC.

When the spike recedes, this memory continues to be retained.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

I'll submit a PR with failing test case and fix shorty.

What is the expected behavior?

When stream is destroyed, it should clean up its resources.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Issue introduced in 961eb65 which is in 16.7.0.

[overlookmotel]

PR to fix: #14706

[overlookmotel]

Actually, I've noticed that this bug can also cause one render's context to leak into another later render. This is likely to be more common and more serious than the original effect outlined above.

Steps to reproduce:

• Render 1 uses .renderToNodeStream(), it gets threadID 1
• Render 1 renders <MyContext.Provider value={'XXX'}>
• Render 1 stream is destroyed before </MyContext.Provider> is reached in the render
• MyContext slot for threadID 1 still contains 'XXX'
• Render 2 later on also gets allocated threadID 1
• Render 2 renders <MyContext.Consumer> without an enclosing provider
• The consumer returns 'XXX' rather than default value
• All later renders using <MyContext.Consumer> without an enclosing provider also receive 'XXX'

So data from render 1 leaks to all subsequent renders which access MyContext without an enclosing provider.

I've added a test for this to PR #14706. That test fails on current master, and I expect this problem also affects v16.7.0.

[overlookmotel]

Issue also affects .renderToString() if an error is thrown during the render, leading it being prematurely destroyed, leaving some contexts open.

Many would say that you should terminate the Node process on an unexpected error, but I imagine a lot of people catch it and send status 500 to the client.

[eliseumds]

Just faced the same problem with renderToString() (sync). I render a bunch of <AppContext namespace={string}> which I later use for things like test IDs and ad unit paths. The second render after a SSR error keeps the same array of app contexts, already filled up from that first problematic request, like so:

Request that explodes:

<AppContext namespace="root">
  <ExplodesHere />
</AppContext>

And the second render, of a page that "works":

<AppContext namespace="root">
  Here I will have two context objects: [{ namespace: 'root' }, { namespace: 'root' }]
</AppContext>

[gaearon]

Thanks @eliseumds for explaining your case, I added a regression test for it.

[gaearon]

Fixed in 16.8.3.