remix icon indicating copy to clipboard operation
remix copied to clipboard

Session docs should be clearer on potential race condition pitfalls when committing session in nested routes

Open johtso opened this issue 2 years ago • 6 comments

What version of Remix are you using?

1.14.0

Are all your remix dependencies & dev-dependencies using the same version?

  • [X] Yes

Steps to Reproduce

Take this example scenario.

You have this route setup:

root
  - index
  - api/login

In root's loader, you consume a flash message from the session and commit it. In index you consume an unrelated flash message from the session and commit it. In api/login's action you write a flash message saying the user has been logged in.

What you end up with now is a race condition where, after logging in, both index and root are reloaded, causing two session commits to happen simultaneously, with unpredictable behaviour (in my case the flash message was never deleted).

I feel like the documentation should mention this potential issue, and maybe describe how to deal with it (I'm still not clear how you're supposed to navigate this).

https://remix.run/docs/en/1.14.0/utils/sessions#session-gotchas

Typically if you're using flash, you'll want to have a single loader read it, if another loader wants a flash message, use a different key for that loader.

This seems misleading, using different keys will not stop commits from stomping over each other if the routes get loaded simultaneously

https://remix.run/docs/en/1.14.0/utils/sessions#createcookiesessionstorage

That means if you session.flash in an action, and then session.get in another, you must commit it for that flashed message to go away. With other session storage strategies you only have to commit it when it's created (the browser cookie doesn't need to change because it doesn't store the session data, just the key to find it elsewhere).

This also seems incorrect. With other types of session storage you still need to commit when making changes, you just don't have to update the cookie if the session wasn't newly created. But in most situations the session may be getting created for the first time so you just always set the cookie to be safe.. right?

Expected Behavior

N/A

Actual Behavior

N/A

johtso avatar Mar 03 '23 16:03 johtso

@johtso interesting insight! I think I might have struggled with the same issue! What we ended up doing was creating a separate cookie storage specifically for some settings: https://remix.run/docs/en/1.14.0/utils/cookies

I know this makes it more complex because you need to commit more in the header, but this does create separations of concerns.

thomasverleye avatar Mar 04 '23 14:03 thomasverleye

@thomasverleye creating a separate session storage for ephemeral session data seems like a good suggestion!

johtso avatar Mar 04 '23 15:03 johtso

Another example situation:

Your root route fetches user information if the user is authenticated and the info hasn't been previously fetched. The route from which the user logged in displays form errors from session flash.

After logging in the current route is reloaded, along with the root route. They both commit session storage 💥

Couldn't Remix at least warn the developer if more than one route is committing a session storage at the same time?

johtso avatar Mar 04 '23 23:03 johtso

The solution I've ended up with now, is to consume all my flash data in the root route, and expose it using a Context provider.

Seems to work great and avoids a lot of these issues!

johtso avatar Mar 05 '23 02:03 johtso

I think there's a potential for more race conditions in saving sessions from different windows/tabs simultaneously. I think there should be an optimistic lock mechanism, perhaps.

I have started a discussion about this here: https://github.com/remix-run/remix/discussions/9169

moltar avatar Mar 31 '24 18:03 moltar

I just wanted to put a message here as I was investigating a related issue for the whole evening. In my case, I was using remix-toast to display a notification. What I observed was that at times I had double notifications. I've discovered that it is related to a loader race condition. When a flashed cookie was read and committed, the request which was committing it did not have time to complete and hence clear its value from the browser; therefore, when data for a different route was requested, browser provided it again to a loader. That's when the app route saw it again and scheduled a duplicated toast message.

screenshot

grundmanise avatar Jul 28 '24 23:07 grundmanise