zemuldo/nextjs-oauth

Race condition in your UserService

ThisIsMissEm opened this issue · 1 comments

Hi! I saw your article via one of the tech newsletters I subscribe to, and whilst reading, I noticed you've a race conditions in your code that creates a user:

findOrCreate: async (oAuthData) => {
    try {
      const user = await User.findOne({ oAuthId: oAuthData.id });
      if (!user) {
        const newUser = new User({oAuthId: oAuthData.id, oAuthData: oAuthData});
        await newUser.save();
        return newUser;
      }
      return user;
    } catch (e) {
      return Error('User not found');
    }
  },

In the code above there is the potential for creating two users that are identical, should the requests hit the two instances of the server at the same time. Essentially if you look in the database queries that get executed, you have:

  1. find from Users where oAuthId equals X
  2. insert into Users (oAuthId, oAuthData)

Now, this is fine if you only have a single instance running, but if you have multiple instances running (which you probably should in production ready projects), then these database operations may appear as either:

  1. find from Users where oAuthId equals X
  2. insert into Users (oAuthId, oAuthData)
  3. find from Users where oAuthId equals X
  4. (Not executed) insert into Users (oAuthId, oAuthData)

Or:

  1. find from Users where oAuthId equals X
  2. find from Users where oAuthId equals X
  3. insert into Users (oAuthId, oAuthData)
  4. insert into Users (oAuthId, oAuthData) — ERROR conflict

In order to prevent this issue, you should look at using an upsert operation, which does the findOrCreate in one single database operation, allowing the database to sequence creations based on non-existence, rather than application code.

You are right! Thanks for pointing this out in such an elaborate and clear manner. I will be sure to update the article and the example in this repo.