The "on demand" Account creation logic on context can be flaky and should use locking

[janus-reith]

Prerequisites

• Yes

Issue Description

User Accounts in the API currently work like this:

1. Authentication happens before on a third party, usually some oauth flow.
2. On the api, there is no specific "register" step by default. Instead, each time the Apollo context for a graphql call is built, it will check if an account exists for that external Id, if not create it on the go: reactioncommerce/api-core/src/util/buildContext.js#L66
3. Account creation certainly takes some time, and requires a roundtrip to the database. In the meantime, further requests can be received by that client, who is unaware wether the api needs to create some account for it or how long that takes. Since at the start of these other requests, an account might still not exist, further invocations for createAccount will start.
4. These further account creations will not succeed, since MongoDB will not allow inserting the same user id multiple times.
   To migitate this, the API does a simple thing: Swallow the error, and continue with the request like it was a normal unauthenticated one: reactioncommerce/api-core/src/util/buildContext.js#L75
   If errrors occur, the client will never know since no error is thrown.
   (And neither does the server, the Logger.error won't actually log and in turn is swallowed by some Apollo Context logic)

This doesn't match well with the expectation of the api client, which might either be a storefront or some other microservice.

Expected result would be: Send five authenticated requests, await response including account being created, receive five responses, dedicated to that authenticated account.
Actual result: Send five authenticated requests, first one creates the Account and returns, up to 4 of the other requests will cause a silent createAccount failure and the client will receive a result as if the requests where send as unauthenticated.

Steps to Reproduce

I made a sandbox which demonstrates this behavior: https://codesandbox.io/s/account-concurrency-locking-demo-50qi4?file=/src/index.ts

1. Navigate to /accounts/someRandomId and repeatedly hit the same url.
   Notice how the array(which would be our DB) will contain several entires/accounts, since the other requests fired before the first one finished.
   Now in the RC api, this would gladly not happen, since MongoDB will deny insertion due to a unique key constraint. Instead, the RC api would do the aforementioned error swallowing and treat the subsequent requests as unauthenticated.
2. Navigate to /accountsWithLock/someOtherRandomId and repeatedly hit the same url.
   Notice how only one entry is created, since a lock is placed on the ID "someOtherRandomId" and subsequent function calls will pick up that now existing entry. The console logs give some insight on the order of things.

Note: Since codesandbox runs this on an actual server, and multiple people might be trying this out at the same time, pick a unique long ID for testing, or simply fork it.

Possible Solution

Add a locking mechanism.
Wait for a lock on the current accountId(or could also be some other auth header, depends on implementation), then, recheck if an account was created in the meantime, else create it.

For simple setups that either have a single server or have sticky proxy sessions(although no guarantee), a builtin solution storing an object in memory might be fine, just like shown in the sandbox above.

For distributed systems, using Redis might be reasonable, Redlock is a default implementation that could do the job.
Since account creation doesn't happen too frequently compared to e.g. a catalog query though, and for each new account, the period where locks are necesary is rather short, the performance gain of redis might not outweight the increased complexity in infrastructure - It could also work fine to just reuse mongodb to keep track of locks.

I assume it would be optimal if the locking mechanism is pluggable, similar to the appEvents here: reactioncommerce/api-core/src/ReactionAPICore.js#L126

Further context

• These concerns will still stay valid with the switch to accounts-js. While technically we could make the account creation imperative instead by hooking up the creation of docs in Accounts to the accounts-js account creation logic which operates on Users same as Meteor Accounts did, we still want to keep the Accounts logic pluggable for other, potentially oauth2-based solutions.
• I was considering to create this in the api-core repo, but most times I'm still not sure wether api-related issues should belong here or there. Feel free to move it.

Versions

latest