I'll check through our session-handling code to see if we ever use this type of call (I think it's add() in BagOStuff) for sessions, and if it seems like it's necessary at that point.

Off my dome, I could see using this type of write when you're worried about propagation delays between data centres. One scenario:

1. Client connects to server in DC1 and gets a session ID AAAA
2. Application server in DC1 unconditionally writes an empty session object with key AAAA to DC1 cluster
3. User authenticates to application server in DC1
4. Application server in DC1 unconditionally writes a session object with user ID with key AAAA to DC1 cluster
5. Client connects to application server in DC2, with session ID AAAA
6. Application server in DC2 reads session with key AAAA from DC2 cluster, gets no value
7. Empty session object with key AAAA propagates from DC1 cluster to DC2 cluster
8. Session object with user ID and key AAAA propagates from DC1 cluster to DC2 cluster
9. Application server in DC2 unconditionally writes empty session object for key AAAA to cluster in DC2
10. Empty session object with key AAAA propagates from DC2 cluster to DC1 cluster

At this point, the user has been logged out in both data centres.

If the application servers use SETNX semantics whenever it's supposed to be creating a new session, the write in DC2 would generate an error, and the app can do some more careful handling.

I'm not sure if we expect to have user sessions cross data centres in this way where there'd be a race between clients hitting application servers and propagation across the database clusters. If not, this probably isn't a problem.

I also don't think it's a problem if we ignore client-supplied session IDs if they come up empty (which seems like a sound idea).

In T222908#5170802, @EvanProdromou wrote:

I'll check through our session-handling code to see if we ever use this type of call (I think it's add() in BagOStuff) for sessions, and if it seems like it's necessary at that point.
Off my dome, I could see using this type of write when you're worried about propagation delays between data centres. One scenario:

1. Client connects to server in DC1 and gets a session ID AAAA
2. Application server in DC1 unconditionally writes an empty session object with key AAAA to DC1 cluster
3. User authenticates to application server in DC1
4. Application server in DC1 unconditionally writes a session object with user ID with key AAAA to DC1 cluster
5. Client connects to application server in DC2, with session ID AAAA
6. Application server in DC2 reads session with key AAAA from DC2 cluster, gets no value
7. Empty session object with key AAAA propagates from DC1 cluster to DC2 cluster
8. Session object with user ID and key AAAA propagates from DC1 cluster to DC2 cluster
9. Application server in DC2 unconditionally writes empty session object for key AAAA to cluster in DC2
10. Empty session object with key AAAA propagates from DC2 cluster to DC1 cluster

This isn't the way that session handling works, is it? I would expect (hope?) that step 6 would basically look like a failed login.

At this point, the user has been logged out in both data centres.
If the application servers use SETNX semantics whenever it's supposed to be creating a new session, the write in DC2 would generate an error, and the app can do some more careful handling.

Only if we're committed to using a consistency level on write that in-lines cross-DC latency. And, if we're using Cassandra's in-built CAS, that latency will be higher still (there are additional round-trips involved in the PAXOS exchange).

I'm not sure if we expect to have user sessions cross data centres in this way where there'd be a race between clients hitting application servers and propagation across the database clusters. If not, this probably isn't a problem.
I also don't think it's a problem if we ignore client-supplied session IDs if they come up empty (which seems like a sound idea).

This task is to check if this is needed now, or ever.

I believe the answer to this is yes, we do need SETNX behavior, which would make the more interesting/relevant question: Do we implement this internally in the BagOStuff (via a simple read to test for existence) , or do we implement a SETNX operation in Kask that uses Cassandra's CAS functionality.

Generally speaking, the former is a pattern vulnerable to race conditions, but during the RFC discussions I had the impression that races here wouldn't be a problem. I'm not sure if this was due to the way concurrency (or the lack thereof) worked, or if a race simply wouldn't result in an error. It's also possible whoever said this was mistaken, or that I misunderstood entirely.

The latter, implementing SETNX (as PUT) in Kask using Cassandra's CAS will guarantee correctness, but comes at the cost of higher latency and lower throughput (behind the scenes this makes use of Paxos, requiring additional round-trips internally to coordinate the operation).

As far as I can tell, the SessionManager code doesn't use the add() interface from BagOStuff.

I can't find any other code in MediaWiki core that uses the add method on session storage, so I think for right now, we can probably live comfortably with a read-then-write in RESTBagOStuff.

That is, if someone decides to use add() for session storage, they're going to get the read-then-write behaviour, and if they need it to be atomic on the server side, we'll deal with it then.

@BPirkle is there anything we need to add here in the code? Maybe just a comment, or if we're feeling ambitious a very-low-priority log warning?

I agree that the existing SessionManager code doesn't use the add() function (or any of the other functions that call it). And add() already has a TODO indicating that it is not atomic, so I think we're good. I'm hesitant to add logging, given that RESTBagOStuff is a general-purpose class that may be used in the future for things other than session storage. Logging that made sense when the class was used for one purpose might be nonsensical in another.

On the larger question about what we need, as far as I can tell from my brief bit of looking, SessionManager just isn't worrying about things like propagation or race conditions. I don't know enough about either SessionManager or our multi data center environment to know if I'm uninformed, or the code is naive, or if this is just nonproblematic in the overall SessionManager architecture. I'd be interested to test, or at least talk through, various failure modes to make sure we aren't missing anything significant.

But from a coding standpoint, unless we find we need to take on larger changes to SessionManager, I don't see anything to do here.

@EvanProdromou I believe there is nothing more to do here. Please mark as resolved if you are happy with that.