Page MenuHomePhabricator

CentralAuth on SQLite is prone to deadlocks when using a separate database
Open, Needs TriagePublic

Description

This is a generalized version of T363284: Deadlock upon logging in in selenium tests with CentralAuth and SQLite, to address the underlying problem in CentralAuth as opposed to fixing the immediate problem in selenium. I wrote a detailed step-by-step investigation of a similar deadlock in T363284#10366655, and I won't repeat much of that here; so, I suggest reading that comment first.

Context

A typical configuration for CentralAuth is to use a separate centralauth database. This is also the recommendation in https://www.mediawiki.org/wiki/Extension:CentralAuth. Therefore, a typical request will likely need to query two databases: the local wiki database, and the centralauth database.

SQLite is the DB type used by MediaWiki-Docker and quickstart. For MW-Docker in particular, I made a guide on how to set up a wiki farm, and updated the CentralAuth instructions for SQLite. You can use these to reproduce the issue locally, if you don't already have a working multi-wiki environment.

Next, keep in mind that SQLite does not have record-level locking. As soon as we start a transaction with BEGIN IMMEDIATE, the whole database is locked. Other threads can't even start their own transactions because of the IMMEDIATE: it makes SQLite consider the transaction as a write transaction, right from the get go, regardless of whether any writes will be performed. This was done to mitigate other deadlocks in T89180 / T93097.

The problem

The above works just fine if you're not making concurrent requests. However, if you do make concurrent requests, you will occasionally see deadlocks. The reason is that certain requests will query the local wiki database first, and others will query the centralauth database first. This alone wouldn't be a problem, but it becomes one as soon as both requests try to query the "other" database (respectively centralauth and local) while the other request is still running; this will result in a deadlock.

Note that this isn't even specific to CentralAuth. However, CentralAuth is perhaps the best example of a separate, shared database that can cause this.

Test case

Reproducing this theoretical bug in the real world is doable, but not trivial. For starters, it requires that the two requests be sent at just the right time. And then, they need to access the two databases in reverse order. You can find more details in T363284#10366655, but I found that it can easily be reproduced by turning on the debug toolbar and disabling the cache. So, make sure you have the following in your LocalSettings.php:

$wgMainCacheType = CACHE_NONE;
$wgMessageCacheType = CACHE_NONE;
$wgParserCacheType = CACHE_NONE;
$wgMainStash = CACHE_NONE;
$wgResourceLoaderMaxage = [
	'versioned' => 0,
	'unversioned' => 0
];
$wgCachePages = false;
ObjectCacheFactory::$localServerCacheClass = EmptyBagOStuff::class;

$wgDebugToolbar = true;

This should be enough to reproduce the issue with concurrent API requests. Again, this is just an example of something that reproduces the bug for me. Your mileage may vary, and the underlying problem goes beyond specific triggers.

We will use selenium in MW core to reproduce this bug more consistently. Note that this is only done for convenience; there's nothing selenium-specific here. Create two tests with the following contents:

first.js
'use strict';

const Api = require( 'wdio-mediawiki/Api' );

describe( 'First', () => {
	it( 'makes an API request', async () => {
		await Api.bot();
	} );
} );
second.js
'use strict';

const Api = require( 'wdio-mediawiki/Api' );
const BlankPage = require( 'wdio-mediawiki/BlankPage' );

describe( 'Second', () => {
	it( 'also makes an API request', async () => {
		await Api.bot();
		await BlankPage.open();
	} );
} );

Here already, the BlankPage.open() in the second test doesn't seem to be necessary; yet, in my local environment it makes the issue happen much more consistently.

Now we run these tests concurrently until they fail. It might take a while. For me, with the BlankPage line, it sometimes takes 10+ attempts to get a deadlock, and sometimes it's on the first try. Again, YMMV.

for i in $(seq 1 100); do echo "Attempt #$i"; DISPLAY= npm run selenium-test -- --specFileRetries 0 --maxInstances 2 --spec first.js --spec second.js || break; done

Eventually, the test will hang for 60 second and fail with a timeout. If you reduce SQLite's busy timeout as described in T363284#10366655, it will instead fail with:

[0-1] Error in "Second.also makes an API request"
Error: invalidjson: No valid JSON response: 
    at /home/emanuele/Git/core/node_modules/mwbot/src/index.js:254:31
    at tryCatcher (/home/emanuele/Git/core/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (/home/emanuele/Git/core/node_modules/bluebird/js/release/promise.js:547:31)
    at Promise._settlePromise (/home/emanuele/Git/core/node_modules/bluebird/js/release/promise.js:604:18)
    at Promise._settlePromise0 (/home/emanuele/Git/core/node_modules/bluebird/js/release/promise.js:649:10)
    at Promise._settlePromises (/home/emanuele/Git/core/node_modules/bluebird/js/release/promise.js:729:18)
    at _drainQueueStep (/home/emanuele/Git/core/node_modules/bluebird/js/release/async.js:93:12)
    at _drainQueue (/home/emanuele/Git/core/node_modules/bluebird/js/release/async.js:86:9)
    at Async._drainQueues (/home/emanuele/Git/core/node_modules/bluebird/js/release/async.js:102:5)
    at Async.drainQueues [as _onImmediate] (/home/emanuele/Git/core/node_modules/bluebird/js/release/async.js:15:14)
    at process.processImmediate (node:internal/timers:483:21)

Solution

I'm not sure, but I think this should be prevented at the RDBMS lib level. Maybe we should enforce a consistent order in resource access with SQLite?

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Not sure what could be done about this at the RDBMS or CentralAuth level. What could be done is having a DB connection pool of one, so when using SQLite requests involving the DB in any way just have to wait until the previous request has been served.

Not sure what could be done about this at the RDBMS or CentralAuth level.

At the CentralAuth level: nothing, I think. It also wouldn't fix the problem for other multi-database setups not involving CA. Unless there was a simple enough fix, but I'm not seeing it; and the few things I can think of are not too different from the general proposal below.

What could be done is having a DB connection pool of one, so when using SQLite requests involving the DB in any way just have to wait until the previous request has been served.

Seems a good idea. Unless I'm missing something, this would be equivalent to the single-database standard setup, where requests involving the DB can't be handled simultaneously (because they will need to lock the same DB). In fact, now that I think about it again, I don't think we have any alternatives to serializing the DB requests, do we?

As soon as we start a transaction with BEGIN IMMEDIATE, the whole database is locked. Other threads can't even start their own transactions because of the IMMEDIATE: it makes SQLite consider the transaction as a write transaction, right from the get go, regardless of whether any writes will be performed. This was done to mitigate other deadlocks in T89180 / T93097.

Note, if the db is in WAL mode, write requests should not block read requests. Write requests should only block other writes (incl begin immediate). (As far as i understand)

So if begin immediate was only used on actual writes it would probably help.

Anyways, I feel like using BEGIN IMMEDIATE everywhere is not the right approach. I feel like a better thing to do would be:

  • use separate SQLITE connections on DB_REPLICA and DB_PRIMARY handles
  • use only normal BEGIN on DB_REPLICA handles
  • Implicit transactions not part of an atomic section on DB_PRIMARY should use normal begin
  • Explicit transactions/atomic sections on DB_PRIMARY should use BEGIN IMMEDIATE
  • If a write query happens when an implicit transaction is open and not part of an atomic section, the implicit transaction is committed, a new transaction just for the write query is started, the write query is done, and then that transaction is commited. We then continue from there.

I think this would significantly reduce the possibilities of deadlocks, since write locks would be held much less often. However it would still ensure that read transactions are never upgraded to a write transaction, which was the original problem that adding BEGIN IMMEDIATE everywhere was trying to solve.

The downside to this, is that it effectively lowers the isolation level that implicit transactions have if write queries are made on that transaction to read committed (But anything with an explicit transaction, and transaction that makes only read queries, should still be isolated at the Serializable level). This seems like a reasonable tradeoff.

[And always be sure to use WAL mode]

Change #1105883 had a related patch set uploaded (by Brian Wolff; author: Brian Wolff):

[mediawiki/core@master] [Proof of concept, DNM] Hold write locks less often in sqlite

https://gerrit.wikimedia.org/r/1105883

I made a hacky version of what i mean as a proof of concept (code is just a hack, not a proper implementation. If it was adopted it would have to be rewritten. Its meant to be just enough to test the idea to see if it solves the problem or creates any new ones).

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1105883

When testing, i could no longer reproduce the deadlock from your steps to reproduce when using my patch to DatabaseSqlite (In my test, i created the centralauth db with pragma journal_mode="WAL". The instructions on wiki seem to create it with journal_mode "delete". I'd recommend wal mode)

Anyways, try it out and see if it makes the deadlock go away

As soon as we start a transaction with BEGIN IMMEDIATE, the whole database is locked. Other threads can't even start their own transactions because of the IMMEDIATE: it makes SQLite consider the transaction as a write transaction, right from the get go, regardless of whether any writes will be performed. This was done to mitigate other deadlocks in T89180 / T93097.

Note, if the db is in WAL mode, write requests should not block read requests. Write requests should only block other writes (incl begin immediate). (As far as i understand)

Yes, but BEGIN IMMEDIATE causes everything to be treated as a write, so WAL doesn't help. (As an aside, and regardless of this, I wonder if we should force use of WAL mode via the installer)

  • Implicit transactions not part of an atomic section on DB_PRIMARY should use normal begin
  • If a write query happens when an implicit transaction is open and not part of an atomic section, the implicit transaction is committed, a new transaction just for the write query is started, the write query is done, and then that transaction is commited. We then continue from there.

By "implicit transaction" are you also including the main transaction for web requests etc? Committing the main transaction prematurely (if an atomic section is started) might be problematic, no? Especially if a rollback is needed later. Or maybe I'm missing something?

The instructions on wiki seem to create it with journal_mode "delete". I'd recommend wal mode

Yup, updated.

Anyways, try it out and see if it makes the deadlock go away

Not only does the deadlock go away, but the test timing is cut in half (from ~6s to ~3s). Less waiting, maybe?

I'll leave the review to MWI as maintainers of Rdbms and deciding how/whether to support this on SQLite.

As for this task, I don't think this is actually a request for supporting CentralAuth on SQLite. It's written that way, but, if I understand the "user journey" here, this is really a request to have a documented and supported way to set up CentralAuth in our local dev environments.

MediaWiki-Docker and Quickstart are both based on sqlite, so that means unless instructed otherwise, that's what people will have.

One solution here would be a tutorial page on setting up MariaDB/MySQL and CentralAuth alongside Quickstart (assuming we can come up with a neat incremental way to do that).

Added a note to the install instructions about lack of SQLite support.

By "implicit transaction" are you also including the main transaction for web requests etc? Committing the main transaction prematurely (if an atomic section is started) might be problematic, no? Especially if a rollback is needed later. Or maybe I'm missing something?

An alternative version of the plan might be to only do this if no writes have happened yet on the implicit transaction. If a write has happened, we have write-lock so we don't need to do anything special. If there are no writes, then there is nothing to rollback. At worse, reopening the transaction means we might have a different read snapshot. In the mysql DB backend we close and reopen the main transaction if the mysql server goes away (and nothing was written) so any code that doesn't work with the implicit transaction possibly not having a consistent read snapshot outside of atomic sections should already be broken under mysql.

As for this task, I don't think this is actually a request for supporting CentralAuth on SQLite. It's written that way, but, if I understand the "user journey" here, this is really a request to have a documented and supported way to set up CentralAuth in our local dev environments.

We do have documentation for setting up CentralAuth locally, at least in MediaWiki-Docker, and I contributed to said documentation by adding SQLite instructions: https://www.mediawiki.org/wiki/MediaWiki-Docker/Extension/CentralAuth. I don't know about Quickstart, though.

It's maybe not very clear from the task description, but the main request in this task is to do something to Rdbms (presumably) that would improve support for multi-database setups on SQLite. CentralAuth being just an example of something that happens to use a secondary database; and possibly the most common/relevant example.

Note that CentralAuth works just fine in SQLite for normal usage. It only starts to break when making concurrent requests; and perhaps the most common example of that in a local environment are selenium tests.

By "implicit transaction" are you also including the main transaction for web requests etc? Committing the main transaction prematurely (if an atomic section is started) might be problematic, no? Especially if a rollback is needed later. Or maybe I'm missing something?

An alternative version of the plan might be to only do this if no writes have happened yet on the implicit transaction. If a write has happened, we have write-lock so we don't need to do anything special. If there are no writes, then there is nothing to rollback. At worse, reopening the transaction means we might have a different read snapshot. In the mysql DB backend we close and reopen the main transaction if the mysql server goes away (and nothing was written) so any code that doesn't work with the implicit transaction possibly not having a consistent read snapshot outside of atomic sections should already be broken under mysql.

This makes sense to me, but it's also getting out of my knowledge zone of the rdbms lib :D