Skip to content
This repository has been archived by the owner on Apr 9, 2022. It is now read-only.

Double Click Register #354

Open
angadgarg25 opened this issue Sep 4, 2019 · 3 comments
Open

Double Click Register #354

angadgarg25 opened this issue Sep 4, 2019 · 3 comments
Labels
Authentication bug Something isn't working

Comments

@angadgarg25
Copy link
Member

If an impatient user (e.g Timothy Ko) clicks the register button twice, two users may get created for them in the auth server. We should avoid this if possible, I'm not sure what the effect of this issue is but it could mess things up.

@angadgarg25 angadgarg25 added bug Something isn't working Authentication labels Sep 4, 2019
@josh-byster
Copy link
Contributor

josh-byster commented Sep 12, 2019

Interesting! Is this functionality you've seen happen? In theory, there shouldn't be any possibility for a user to be created twice in the auth server. We do a check beforehand to make sure this doesn't happen. Though node is single-threaded, we can still run into race conditions with concurrent DB access, since accesses are not happening within a single turn of the event loop. I'm thinking this is what's happening:

  1. First register request comes through. find returns nothing since no value exists yet for the user.
  2. JWT gets asynchronously generated. The rest of the method (including the save to the DB) is put "on hold" due to the await, as it should.
  3. Second register request comes through before first one completes. Same thing happens: find returns nothing since the first user hasn't been saved.
  4. JWT gets asynchronously generated.
  5. Now that the JWT generation is done for the first register request, the remainder of the method is put back onto the event loop to be processed.
  6. The first user.save() gets called, so it persists a document to the DB.
  7. JWT generation for the second request is done, so the same thing as step 6 happens for the second request.
  8. The second user.save() gets called, so it persists a new document to the DB.
  9. Now we have 2 users with the same info in the DB :'(

We want to avoid synchronous operations as much as possible but we may want to look into using async-lock for this. I'll do more research as well to see how Node experts would handle it.

Essentially I'm almost certain a situation like this is what we're looking at.

@angadgarg25
Copy link
Member Author

@josh-byster yup it happened when tim signed up lol

@josh-byster
Copy link
Contributor

josh-byster commented Sep 12, 2019

Oof, glad we discovered this bug early. It certainly should not be the responsibility of the front-end developers to make sure the "Register" button gets hit only once. We have to synchronize these requests somehow and I have to look into the best way to do that. I'll link this to an issue in infra server and hopefully push up a fix soon. I want to do more research to figure out the best approach to making these updates happen either atomically or synchronously.

Will you be able to grab the changes if the source gets updated on that repo? I could push up a fresh Docker image as well. As a side note it'd probably be best to have that infra server packaged as a Docker instance as possible so that new changes can get populated to the consumers of the server by just pulling a new image version.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Authentication bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants