From e82e7b3f9f8a8f82f90fa9c50a17dd5698e04d9d Mon Sep 17 00:00:00 2001 From: Bryan Date: Sat, 22 Jan 2022 12:48:37 -0800 Subject: [PATCH] fix: Login - fix /api/v2/user cache race condition (#47) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While I was testing the Sign-out functionality in #46 , I found a bug on login. Sometimes, intermittently, the login wouldn't 'take' even though the login POST was successful and the subsequent GET to `/api/v2/user` was successful - the user would still be brought to the `/login` screen a second time: ![2022-01-21 19 52 31](https://user-images.githubusercontent.com/88213859/150623831-7ce33fca-97d5-4ea8-8301-9149def1ee40.gif) ^ I log-in, and then the login screen just resets, and logging in a second time works 🤔 I tracked this down and found it is a new race condition that I inadvertently introduced when I migrated to SWR in #46. Because SWR caches responses based on the API path - we have to invalidate the cache for `/api/v2/user` when the user logins, otherwise we'll continue to serve the user-not-found error until SWR decides to retry. However, we weren't waiting for that cache-invalidation to go through - so there was the chance that an `/api/v2/user` request was still in-flight while we were redirecting after a successful login. If the request made it back prior to the redirect - login would work on the first try. If the request took longer - SWR would serve the stale, erroneous user from the cache. Luckily the fix is simple - `mutate` in the SWR API returns a promise, so we can just wait for that to go through before redirecting. Unfortunately, though, this is tricky to exercise in a unit test because with the SWR model, the login logic is spread between `_app.tsx`, `UserContext.tsx`, and `SignInForm.tsx`, and the race condition involves an integration of all of those places. I found that there is a lint rule that can help protect us from making this mistake - [`typescript-eslint/no-floating-promises`](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-floating-promises.md) (it's a lint rule we wanted to turn on in `coder/m`, too, actually - just a lot of work to clean up the code for it!), so I turned it on as part of this PR, and it caught a couple extra places to check. --- .eslintrc.yaml | 1 + site/components/SignIn/SignInForm.tsx | 4 ++-- site/contexts/UserContext.tsx | 4 +++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.eslintrc.yaml b/.eslintrc.yaml index 0443a81bbb..f9ef172e06 100644 --- a/.eslintrc.yaml +++ b/.eslintrc.yaml @@ -38,6 +38,7 @@ rules: "@typescript-eslint/explicit-function-return-type": "off" "@typescript-eslint/explicit-module-boundary-types": "error" "@typescript-eslint/method-signature-style": ["error", "property"] + "@typescript-eslint/no-floating-promises": error "@typescript-eslint/no-invalid-void-type": error # We're disabling the `no-namespace` rule to use a pattern of defining an interface, # and then defining functions that operate on that data via namespace. This is helpful for diff --git a/site/components/SignIn/SignInForm.tsx b/site/components/SignIn/SignInForm.tsx index b66735c5da..4299f33667 100644 --- a/site/components/SignIn/SignInForm.tsx +++ b/site/components/SignIn/SignInForm.tsx @@ -87,8 +87,8 @@ export const SignInForm: React.FC = ({ try { await loginHandler(email, password) // Tell SWR to invalidate the cache for the user endpoint - mutate("/api/v2/user") - router.push("/") + await mutate("/api/v2/user") + await router.push("/") } catch (err) { helpers.setFieldError("password", "The username or password is incorrect.") } diff --git a/site/contexts/UserContext.tsx b/site/contexts/UserContext.tsx index 544d5dd835..a7885d2e47 100644 --- a/site/contexts/UserContext.tsx +++ b/site/contexts/UserContext.tsx @@ -23,7 +23,9 @@ export const useUser = (redirectOnError = false): UserContext => { const requestError = ctx.error useEffect(() => { if (redirectOnError && requestError) { - router.push({ + // 'void' means we are ignoring handling the promise returned + // from router.push (and lets the linter know we're OK with that!) + void router.push({ pathname: "/login", query: { redirect: router.asPath,