Page MenuHomePhabricator

Two-level session storage and the consistency problem with serialized blob stores
Open, Needs TriagePublic

Description

Sessions implement their own little session-local key-value store as a serialized blob. The blob is read at the start of the request and written at the end of the request, leaving an enormous critical section for concurrency issues. If one request writes one key, and a different concurrent request writes another key, whichever writes the blob last will win and the other key will disappear.

Session::set() says:

	public function set( $key, $value ) {
		$data = &$this->backend->getData();
		if ( !array_key_exists( $key, $data ) || $data[$key] !== $value ) {
			$data[$key] = $value;
			$this->backend->dirty();
		}
	}

For example, if you log in, and then open two edit pages in two different tabs sufficiently quickly, you will always get a CSRF error when submitting one of them. I have reproduced this by inserting a sleep at the end of SessionBackend::__construct().

Really it would be better to have a two-level key/value store. Session::set() would just write to the backend immediately with a key that is composed of the session ID and $key.

There are some groups of keys that should probably be written together, for example wsUserID and wsUserName. These could be in a single blob written with a single Session::set() call.

Session::get() would still read and validate the session metadata before fetching a subkey, and so would still be able to implement O(1) invalidation of subkeys of a session. We could also use a session store that supports bulk deletions.

(I've talked about this problem a few times over the years, but I don't think I've properly documented it before. Related discussions are at T267270 and T270225.)

Event Timeline

Possibly related is https://gerrit.wikimedia.org/r/c/mediawiki/core/+/659617 (Last-Write-Wins updates for subkeys within a key).

Ref T158365: Session "{session}": Metadata merge failed: {exception} and T204459: Session "{session}": CentralAuth saved source {saved} != expected source {expected}.

I'm not very familiar with the background of that error, but it seems like perhaps this is a symptom of combined writes losing out?

Ref T158365: Session "{session}": Metadata merge failed: {exception} and T204459: Session "{session}": CentralAuth saved source {saved} != expected source {expected}.

I'm not very familiar with the background of that error, but it seems like perhaps this is a symptom of combined writes losing out?

For session metadata (as opposed to session data), atomic writes are actually preferable. These errors occur because the stored metadata mismatches the cookie. More frequent writes could make that better or worse for mostly-accidental reasons, but overall I don't think there is not much overlap with the subject of this task.

T394075: Investigate using different stores for different kinds of sessions proposes a session store abstraction which knows about itself that it is a session store. That would allow storing session fields as separate items.

There are some groups of keys that should probably be written together, for example wsUserID and wsUserName.

We should get rid of those, they just duplicate information that's already in the metadata part of the session blob.

I think the main problem with this (assuming a pure key-value store) would be session ID reset, which requires copying all the data from the old session to the new session.

Also per {T390514} the number of writes seems like a bottleneck, and this proposal would increase it significantly. (Although in other ways smaller separate blobs would improve storage space use in a sessionstore with a binlog, so it's possible it would be positive on the net.)

Also per {T390514} the number of writes seems like a bottleneck, and this proposal would increase it significantly. (Although in other ways smaller separate blobs would improve storage space use in a sessionstore with a binlog, so it's possible it would be positive on the net.)

T390514 was an example of the workload changing to something well beyond what the cluster was sized for. sessionstore will scale linearly, so I'm sure we can accommodate any increase in storage and/or throughput if there is a need (and with some advanced planning of course).