apollo-client icon indicating copy to clipboard operation
apollo-client copied to clipboard

`useFragment` debounce logic clears timer ID `0` and unintentionally cancels unrelated timers

Open clangen opened this issue 7 months ago • 8 comments

Issue Description

The useFragment hook uses a simple debounce mechanism by calling clearTimeout(lastTimeout) before lastTimeout has ever been set. Since lastTimeout is initialized to 0, the very first invocation ends up calling clearTimeout(0). In environments like React Native (Hermes or JSC), timer IDs are drawn from a shared numeric namespace (0, 1, 2, …). Clearing ID 0 thus cancels any timer that was assigned handle 0—often the first setInterval or setTimeout created by user code—leading to silent and hard-to-trace timer cancellations.

See here: https://github.com/apollographql/apollo-client/blob/ee9c6a6ccd273e325c6316da6021afd895c20e74/src/react/hooks/useFragment.ts#L144

Link to Reproduction

N/A

Reproduction Steps

  1. In a React Native app, create a repeating timer before mounting any component that uses useFragment:
const id = setInterval(() => console.log('tick'), 1000);
  1. Render a component which uses useFragment from Apollo Client.
  2. Trigger an update that causes the fragment subscription’s next handler to run.
  3. Observe that immediately after the first fragment update, your interval stops—because clearTimeout(0) was called.

@apollo/client version

3.11.8

clangen avatar May 20 '25 00:05 clangen

Oof, that's a bummer.

The problem is that setTimeout and clearTimeout are not part of the JavaScript specification, but of the HTML Standard - and Hermes probably doesn't stick to that too closely 😕

If they would go by the standard, it clearly states in timer initialization steps

If previousId was given, let id be previousId; otherwise, let id be an implementation-defined integer that is greater than zero and does not already exist in global's map of setTimeout and setInterval IDs.

with an emphasis of "greater than zero".

That said, it seems that Hermes starts nextTaskId at 1, not at 0.

https://github.com/facebook/hermes/blob/3cc0974e4cb1afa7fbc54e454819aad8316e23a0/include/hermes/ConsoleHost/ConsoleHost.h#L38

It also checks if a task is actually present before randomly removing things: https://github.com/facebook/hermes/blob/3cc0974e4cb1afa7fbc54e454819aad8316e23a0/include/hermes/ConsoleHost/ConsoleHost.h#L62

Looking at JSC, they will also never return 0 as a timer id:

https://github.com/WebKit/WebKit/blob/2a64f298232389bf262c7d85229c12dba360fea1/Source/WebCore/page/DOMTimer.cpp#L208

https://github.com/WebKit/WebKit/blob/2a64f298232389bf262c7d85229c12dba360fea1/Source/WebCore/dom/ScriptExecutionContext.cpp#L565-L571

React Native also implements their own timers but they also start GUID at 1

https://github.com/facebook/react-native/blob/a6e854efe9d34c98e9e978ddba0a223373ec8b3e/packages/react-native/Libraries/Core/Timers/JSTimers.js#L58-L65


I'm sorry that I have to ask this and don't mean any disrespect, but have you actually experienced this happening? We're seeing an influx of "plausible, but not correct" AI-generated bug reports in Open Source repos recently, so I just want to double-verify before I put more time into this. It's a weird world we live in 😕

phryneas avatar May 20 '25 13:05 phryneas

Yeah, we are observing this happening.

In our app, very early in the init process, we start start a recurring timer via setInterval when in __DEV__ mode to periodically report runtime stats to some internal tooling that we use to measure performance.

After upgrading from RN 0.72 to RN 0.76 (with bridgeless), we noticed that the timer would fire a few times, then just stop.

It took about a day of debugging to figure out what was going on, and I finally traced it back to useFragment calling clearTimeout(0) after monkey-patching clearTimeout, clearInterval, and setInterval. Unfortunately due to work security policies I cannot develop against the rn-apollo-client-testbed as it uses ngrok so I'm struggling to get an example app with the required template up and running.

In the mean time, here's a screenshot showing what we're observing. You can see we call setInterval() before booting the main part of our app. The value returned is 0. Then, later, clearTimeout(0) is called, which cancels the interval:

Image

For the sake of completeness, here's a screenshot that shows the calling code:

Image

I did go back and test RN 0.72 and can see that the value 1 is returned for the first invocation of setInterval, so perhaps it's some bug that is specific to certain react native versions (or maybe strictly related to bridgeless mode, or new architecture).

At any rate, I think it may be a good idea to be defensive in the Apollo Client, even if the value 0 is supposed to never be a valid timer identifier. That code in question could just set the initial value of lastTimeout to undefined and not call clearTimeout() unless the value is defined.

clangen avatar May 20 '25 18:05 clangen

Thank you for verifying!

I can reproduce this in a new expo project with a custom entry point like this (hooked up as main entry point in package.json):

const timeout1 = setTimeout(() => {
  console.log("Timeout 1");
}, 100);
const timeout2 = setTimeout(() => {
  console.log("Timeout 2");
}, 200);
const timeout3 = setTimeout(() => {
  console.log("Timeout 3");
}, 300);
console.log({ timeout1, timeout2, timeout3 });
clearTimeout(0);
require("expo-router/entry");

Full repo: https://github.com/phryneas/rn-setTimeout-id-0-reproduction

That is very unfortunate :/

We're currently a bit "between versions" as we're in the final stretches of getting 4.0 out of the door, but I'll later discuss if we can bring a fix for this back into 3.x.

That said, this is an upstream React Native bug. This is spec-violating behaviour, so it should probably be reported there, too. Just not sure which repo is right here - all code I've seen indicates 1 as a starting point.

phryneas avatar May 21 '25 08:05 phryneas

Note: I've forwarded this to @kadikraman at Expo, who could reproduce it and is forwarding it further - so this should get fixed upstream, too.

phryneas avatar May 21 '25 11:05 phryneas

See https://github.com/facebook/react-native/pull/51500 :)

phryneas avatar May 21 '25 14:05 phryneas

Awesome, thank you for the update! Looking forward to the upstream fixes as well.

For anyone who may be experiencing this issue, a very quick, very dirty workaround is to just schedule a dummy timeout in your app's entry point before any dependencies or any other code runs.

clangen avatar May 21 '25 23:05 clangen

Just as another update from our side - we also discussed this internally.

Right now we're not going to patch this on our side, but wait and see. We expect that this fix will probably get backported on the React Native side, so it should be available to everyone via a patch update. And at that point, a patch update on our side wouldn't make much sense - you'd have to be in a scenario where you do update Apollo Client, but not React Native, to get a benefit from this, which seems unlikely.

If this fix doesn't get backported and there are still supported React Native versions keeping this bug, we can still think about getting a patch release of AC out.

That said, either way we very much encourage everyone to update their React native version once the bugfix for this is out. This bug doesn't only affect the existing behavior in Apollo Client, but can also lead to bugs like this in userland code:

let intervalId = undefined

function elsewhere(){
    // 0 is falsy, this will not happen for the first interval
	if (intervalId) clearInterval(intervalId)
    intervalId = setInterval(....)
}

// first interval will be started
elsewhere()
// but never be cleaned up before the second interval is scheduled
elsewhere()

phryneas avatar May 22 '25 10:05 phryneas

As per the referenced commits above, this should be resolved in React Native 0.79.3. Additional pick requests to backport to 0.78 and 0.77 exist but haven't been accepted and published yet

kitten avatar Jun 10 '25 15:06 kitten

Hey all 👋

Since this fix has been addressed in React Native and we haven't seen additional activity in the last 2 months, I'm going to go ahead and close this. Appreciate the report and thanks to @kitten for the assist!

jerelmiller avatar Aug 26 '25 23:08 jerelmiller

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

github-actions[bot] avatar Aug 26 '25 23:08 github-actions[bot]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. For general questions, we recommend using our Community Forum or Stack Overflow.

github-actions[bot] avatar Sep 26 '25 00:09 github-actions[bot]