[Bug]: Fetcher Revalidation Race Condition (Remix)

[brophdawg11]

What version of React Router are you using?

6.11.0

Steps to Reproduce

https://discord.com/channels/@me/1105622443453857842/1105623069365649458

It seems there's a race condition in Remix when the fetcher.load kicks off on initial render, and then the subsequent render-driven navigate triggers a shouldRevalidate call on the fetcher - and if the fetcher has not yet loaded the route module for the Remix route it bombs out.

The fetcher probably shouldn't attempt to revalidate at all since it's tied to a hardcoded route. With navigational loaders even on reused routes we still want to call shouldRevalidate so folks can opt-in, but with fetchers they can manually re-trigger via fetcher.load if necessary.

I do worry this fix might still have the same race condition bug if the render-driven navigation comes from useSubmit- since fetchers should attempt to revalidate after actions. But really GET-driven initial renders shouldn't trigger mutations. Worth digging a bit deeper into the Remix side of things to see if there's any avenues to look into there.

Expected Behavior

Fetcher does not try to revalidate and does not call shouldRevalidate

Actual Behavior

Fetcher is trying to revalidate before the route module loads.

[cmerrick]

@brophdawg11 I believe we're seeing this in our application, but the discord link with reproduction steps isn't accessible. Can you post the reproduction steps? Thanks!

[brophdawg11]

The reproduction is here but I'm not sure it quite reproduces on stackblitz because they serve JS files from a service worker so we don't hit the underlying race condition: https://stackblitz.com/edit/remix-run-remix-qdy2d2

[cmerrick]

Confirmed I was able to reproduce using that code once I activated Network Throttling in chrome (set to 'Slow 3G'). Error looks like this:

[n8agrin]

@brophdawg11 I hacked together a "fix" in my fork of Remix: n8agrin/remix#1

The pattern I'm noticing is that in many contexts (shouldRevalidate being just one), Remix attempts to access routeModules assuming that all possible required modules are always loaded. We're seeing that's not always the case in this issue and in the issue I'm trying to fix in this other PR: remix-run/remix#6409

My unsolicited 2c is that access to routeModules should be behind an interface that can: retrieve from local cache, load from the network or throw.

I'm trying to begin to see what that would feel like in this pr, and I don't love how the async nature of the problem demands more and more of the code be refactored to manage one small dependency.

Note that I tried the reload approach (as in remix-run/remix#6409), but that doesn't work. It creates an infinite loop within the shouldRevalide call (no surprise I suppose).

Do you have any ideas? Maybe we need to bite the bullet and do the async refactor.

[n8agrin]

@brophdawg11 just realized some of my edits span react-router and they aren’t captured here. I’ll upload the diff later. In particular I changed the createShouldRevalidate generator to async load a route module if it’s not found, which solves this issue

[brophdawg11]

This is fixed by #10623 and should be available in the next release.

[brophdawg11]

@n8agrin I don't think we want to make shouldRevalidate async - it's intended to be a synchronous decision on whether or not to proceed with the async revalidation. The bug here wasn't that we expected the route module to be loaded - it was instead that this wasn't a "revalidation" at all since no initial load have ever completed, so it's just an initial load and doesn't need to be checking shouldRevalidate at all.

[n8agrin]

@brophdawg11 let me try to explain the bug we're seeing. It's not readily clear that the fix for this issue will fix our particular problem:

• occasionally, loaders will return undefined
• we possibly connected the problem to this issue. We're not 100% certain, but it has similar symptoms
• when i traced the code via my repro case the code seems to be trying to load a route module that is not in the cache
• this seems to tie into the shouldRevalidate logic
• I believe it's not the first time the route logic for the loader is exercised. Our e2e test that hits this issue tends to trigger it deep in the test, well after other actions should have triggered the same loader

If I understand your patch correctly, it doesn't trigger shouldRevalidate on initial route load, because the routeModule may not yet be loaded. My mental model about how this works is confused, because it seems like the synchronous logic assumes all of the route modules must be loaded, when they clearly may not be.

Two questions:

1. Based on the description of the behavior of our bug, do you think this patch will fix the issue? (I will also attempt to test on a build with this patch)
2. Can you help me refine my mental model about how Router / Remix handle route modules, and in particular the interaction between synchronous code paths managing assets available asynchronously?

[github-actions]

🤖 Hello there,

We just published version 6.14.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

[brophdawg11]

It's hard to speculate on your specific root cause without a reproduction. If it's related to a fetcher.load followed very quickly by a navigation then it could be this same root cause.

The mental model of "data routing" boils down to data loading and rendering being completely decoupled. All data loading completes before we even inform the react layer of the matched components for the new route. In Remix, part of the "load" step is loading the route module and putting it in the route modules cache. UI concerns, such as rendering the matches don't happen for new routes unless the load was successful. If the route module fails to load, Remix performs a hard refresh via window.location.reload(). So we should't be trying to make every access to the route module async so it can handle a load, because if we've gotten to the rendering step without a route module, something is already wrong. If we were to load route modules at the access point we'd be introducing network waterfalls.

shouldRevalidate is a UI concern, just like rendering. It's asking the question - should I update the stuff that's already been loaded and is already displayed. If nothing's yet been loaded, then there's no question to answer and we should just load the freshest data.

[n8agrin]

@brophdawg11 awesome thanks for that

[n8agrin]

@brophdawg11 this didn't seem to fix our issue. I'll try to craft a repro, post it and/or provide a patch

[github-actions]

🤖 Hello there,

We just published version 6.14.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!