Handle `_rev`to work with "Eventually Consistent" model.

[ghost]

I've got an app that uses this module and just spent several hours debugging a race condition in our login process. We have a workaround in place, but an update to this module could help prevent it for others.

Essentially, our race condition was caused by having the /login/postResponse endpoint and /whoamiendpoint called in rapid succession. Here was the sequence:

• Client has an unauthenticated session.
• POST /login/postReponse request comes in with authentication from SSO.
• Authenticated user data is added to req.session.
• At the end of the request, the now-authenticated session is saved and client is redirected to /.
• GET /whoami request comes in. (Web UI script finding out who the user is.)
• The authenticated session hasn't finished writing to Cloudant yet, so the unauthenticated session is returned.
• At the end of the request, the unauthenticated session is saved -- overwritting the authenticated one -- and the client is told they are not logged in.
• Repeat until Cloudant wins the race and the authenticated session isn't overwritten.

I should note that this undesirable behavior is not caused by connect-cloudant-store. Instead, it's a result of Cloudant's eventually consistent model and the fact that we don't use server-side rendering. Because our client calls /login and /whoami in close succession, we're always going to have that race condition unless we explicitly inject a delay to give Cloudant time to write.

However, this bug was very difficult to debug, because there were no errors thrown by express-session or connect-cloudant-store. Since the prototype.set function looks up the current _rev field, instead of using the one from when the session was loaded, it silently overwrites the session in the database. This made it appear as though the authenticated session was never being written, when it was actually just being immediately overwritten with the older, unauthenticated one.

I propose that we remove the db.head call on connect-cloudant-store.js#L133 and use the _rev field as it was when the session was loaded. This will cause document collisions to be detected and logged with errors, instead of silently overwriting.

Though I would suggest this be the default behavior, we could make it configurable instead. Potentially leave the existing behavior as default and add a preventOverwrite option to change it.

In either case, I would be happy to do the work myself and submit a pull request with the desired changes.

[adriantanasa]

@jftanner By dropping the reading of the _rev value (the head call) in the set method at line 133, the application will have to store the revision value in memory and rely on that being updated programatically. From my experience of using this module in an multi-instance application the result is to have more conflicts as the session will be updated in another subprocess while still in use in others.

The current implementation is allowing the last action on session save to update the value of the content. The application should take care of blocking the reset of the apis until authorised session is issued. This replicates the model for other storage engines as Redis.

There is however a very good point on the debugging support for this kind of issues. I think the module will benefit from a debug message when the local revision is replaced by the current one in database so the developer could address the delay required on the authorisation task.

[ghost]

According to all the recommendations and guides I read, when express-session is properly implemented, the session should only be generated/used on non-static assets. (Otherwise you generate a bunch of sessions when first loading a page, due to race conditions.) So, in theory, the session should only be read by a single instance of the app at any given time.

Of, course, that assumes a lot of best practices. If your client makes multiple simultaneous or closely consecutive requests, you can start getting a lot of conflict errors just like you say.

The problem is that, with Cloudant, there's no reasonable way to block until the session is issued. Because Cloudant uses the "Eventually Consistent" model, the document is not yet saved when the request is complete. Worse, there's no way to know when the save has been successful. In theory, you could repeatedly request the session again until it returns the new value, but that is incredibly inefficient.

So, I stand by my recommendation of making the _rev replacing optional. Though, since you're right about it potentially causing complications in multi-instance apps, I suggest leaving the default behavior as it currently is.

[ghost]

Just making a note that I haven't forgotten about this. This month has just been real busy on other projects. I might have to do this in my off-hours.

[adriantanasa]

Closing feature request as no progress here.

[ghost]

That's fair.

I still encounter this issue a lot, but there's just been too much other things to do. Ultimately, the right answer is probably to just not use an eventually-consistent DB for sessions.