Bug: Cache in SSR

[HsuTing]

I found requestCache caused a bug. In server side, componentDidUpdate never be called. As a result, requestCache never be cleaned.

I used react-relay-network-modern-ssr in my project and this was ok when I loaded the website for the first time. This queried data correctly. However, after I reload the website, this did not request again to get the newest data.

Because nodejs never import each module again, requestCache will never be cleaned in server.

[jstejada]

hey @HsuTing , which version are you using?

[HsuTing]

@jstejada react-relay@^1.7.0 Do you need the other version of the packages?

[sibelius]

Can you try with 2.0.0-rc1?

[HsuTing]

Same problem

[sibelius]

@HsuTing can you try to come up with a failing test ?

[robrichard]

Could this request cache also potentially leak data to unauthorized users? If the same query is issued with different environments with different authorization levels, the user with less authorization could potentially get the cached response from the user with higher authorization?

[HsuTing]

@sibelius OK, I will try to make a simple example. @robrichard Yes. Users can not get the right data after they login. The data is locked by the cache response.

[HsuTing]

@sibelius Here is a simple example. If you need the more information, let me know 😸 .

[crash7]

Hi all! I can confirm the issue too with a different implementation of Relay SSR than the used in @HsuTing example.

The real problem is that the requestCache[key] is only cleared in componentDidMount, which never happens on SSR: https://github.com/facebook/relay/blob/master/packages/react-relay/modern/ReactRelayQueryRenderer.js#L189-L194

Because of that, the "request" is always in flight: https://github.com/facebook/relay/blob/master/packages/react-relay/modern/ReactRelayQueryRenderer.js#L365

Would be nice to have a way to force that fetch someway, maybe with an extra prop in the QueryRender component that I can set when process.browser is false.

[m64253]

I think having "state" like this outside of the component instance is always going to an issue server side?

Maybe this can be moved to the network?

Would love to help to resolve this but I'm a bit uncertain on what the actual problem being solved by requestCache is, simultaneous request duplication?

[edvinerikson]

Just speculating..

Looking at the code, this seem to try fixing using side-effects inside a method (constructor) which should not have such side-effects. I believe the problem attempted to solve occurs when you run the code in strictmode in development.

It really seem to be a non-problem (double call of constructor) in production but may perhaps look odd in development, I don't know if anyone at FB can confirm this or not? @flarnie, perhaps?

By design this implementation is correct as it tries to de-dupe requests made by different react component instances, though I am not sure it's a feasible optimisation to do given the effects it has in server environments. For the record, there is a similar bug like this existing today in the React Context implementation as well (https://github.com/facebook/react/pull/13874).

One solution to this may be to clear the cache in getDerivedStateFromProps. It's executed immediately after the constructor on the server.. Its not supported but the "side-effects free methods" ship has already sailed so..

Still to me this sounds like a fix to a problem that has far more significant side-effects than what it solves currently (given the context from a outside contributor.. perhaps more details is provided internally).

Maybe someone from React team can help out suggesting a better way here? CC @gaearon @sebmarkbage

Edit

As @m64253 says, perhaps this can be solved on a application level by offering de-duping in the network layer provided by app developers. It may not be a great DX when using strictmode but at least it doesn't affect production runtimes.

[crash7]

Now that @flarnie left facebook[1], who will help us? :scream: :scream:

[1] https://twitter.com/ProvablyFlarnie/status/1081365351139467266

[josephsavona]

We’re actively working on suspense-compatible APIs, which will include addressing issues such as these. Stay tuned!

[holmesal]

We've been struggling with this as well.

The only way we've been able to ensure that the query is actually fetched under SSR is to include a nonce variable (variables are used as part of the cache key) - but this creates a memory leak, as the cache grows with every request and is never cleared.

@josephsavona any progress that could help us out? We're planning to fork ReactRelayQueryRenderer to fix our immediate issue, but would be good to have a proper fix on the Relay side!

[sibelius]

this is fixed on relay-hooks https://github.com/relay-tools/relay-hooks/issues/7

you can try that for now

[sibelius]

can you try the new hooks version https://github.com/facebook/relay/commit/b83aace7a95f5fd82cbb30d1f6888bcc4767eeb5?

[psaia]

It's not a long-term solution but here's a fork which works for our needs: https://github.com/psaia/relay

Relevant change: https://github.com/psaia/relay/commit/96a62eb5dcb9ad8df7a30142255a450934f7fbe8

Hope it helps. Looking forward to the long-term fix.

[sibelius]

is this fixed on relay 9?

[istarkov]

Not fixed, still huge memory leak

[maraisr]

I actually fixed this issue here: https://github.com/facebook/relay/pull/3014

I've been running Relay 9 in production for about a month since this was merged, and it hasn't been an issue. That isnt saying its not working for you! Lets see if we can solve this.

If you run the memory snapshot tool within Chrome's remote attach, you'll probably find where the bug is that you're seeing. Run a snapshot on a fresh start of the page - then snapshot again once you refresh. Then check the snapshot diff between the 2 runs, you'll see the utility has a "GCed delta" something - which is a number of objects retained between them. Investigate those, and see where it is, or what it is.

However; what I have found in the past is that if you're re-using RelayEnvironments between different SSR requests, then that could lead to a perceived memory leak. But is just the store growing. Typically with Relay you'd want a clean Environment for every request.

With some assumptions, eg:

// relay.js
let environment = null;
function getEnvironment() {
    if (!process.browser) return new Environment(...);

    if (environment === null) {
        environment = new Environment(...);
    }

    return environment;
}

// myProvider.jsx
<RelayEnvironmentProvider environment={getEnvironment()}>
    {children}
</RelayEnvironmentProvider>

[istarkov]

Its experimental fix, QueryRenderer cache is still object and can grow until sigabort

see https://github.com/facebook/relay/blob/master/packages/react-relay/ReactRelayQueryRenderer.js#L58

[maraisr]

Ah perfect find @istarkov ✅

Looks like it is cleaning up after itself. 🤔 maybe just needs another delete requestCache in the componentWillUnmount. Are you able to apply this, and see if that fixes it?

Perhaps we need to apply a similar WeakMap trick around the Environment like my other experimental PR did?

Let me know how you go - if not I can have a look at this this weekend ❤

[maraisr]

as a side question @istarkov how are you awaiting the data fetching from the QueryRenderer? Afaik; that component doesnt "block" till data is ready, so the render method will only be called once, and will be the loading state? Unless youre hositing all queries to the root, hydrating the store, then firing the renders?

[istarkov]

I'm not awaiting, just some code is run client-side only. It wasn't an issue for 1+ year for us until our traffic suddenly become 30 req/s from like 0.1 req/sec. (We use it mostly to update client dependent information to have SSR aware of user details) And we get that Query Renderer leaks ;-) So now we wrapped it. Probably it should use weakmap too, or warn once at dev mode about server side usage. (In our case we just check on typeof window)

[maraisr]

If that is client only, then i urge you to try and make that only execute on the client - will solve the memory leak issue, at least for the interim.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.