We've talked about this briefly today in TechCom. We agreed we don't need an RFC for this, however the current commit is lacking a bit of design documentation. It would be good to lay out there (or on Phabricator first) a problem statement and evaluation of existing interfaces we have for locking in filebackend, bagostuff, poolcounter etc., their issues and what this would address, the guarantees it is meant to provide etc.

In addition to the interface itself, it would also be good to have one or two existing callers changed in a draft commit to better show how this would improve certain aspects.

Per last weeks' ArchCom meeting, we'll be tracking this as an RFC after all. My apologies for the confusion.

We'll try to keep the process lightweight. Mainly this means the problem statement and proposed approach should (also) be described in this task's description instead of only in Gerrit.

This RFC has been scheduled for public discussion on IRC next Wednesday, September 20.

As always, the discussion will take place in the IRC channel #wikimedia-office on Wednesday 21:00 UTC (2pm PDT, 23:00 CEST).

I asked @aaron about an example use for this interface, in preparation for the IRC meeting tonight..

<Krinkle> what would be a good example in core or an extension that would make use of this interface (e.g. something that currently does something less nice)
<AaronSchulz> I think the edit stash locks that use always GET_LOCK atm. This is less flexible and is slower for postgres and sqlite since that just does a looping check.
<AaronSchulz> the getScopedLock calls in AuthManager/MessageCache too
<AaronSchulz> ChronologyProtector too

Existing abstract lock managers in MediaWiki core:

@Krinkle thank you for the overview!

I'm still very confused about the name. "InterruptMutexManager" sounds like a manager of mutexes of interrupts. If I understand correctly, "Interrupt" in the name refers to the guarantee that the lock will be released automatically when the current request terminates, no matter how it terminates. So perhaps "AutoReleaseMutexManager" would be more descriptive. But then, will we have MutexManagers that do not guarantee this? Why not just call the interface "MutexManager"?

This RFC was discussed on IRC on September 20th. Log: https://tools.wmflabs.org/meetbot/wikimedia-office/2017/wikimedia-office.2017-09-20-21.01.log.html

During that meeting, it was agreed to let this RFC enter the Last Call period. Should no pertinent concerns remain unaddressed by October 5, this RFC will be approved for implementation.

This was approved in the 2017-10-04 TechCom private meeting.

Change 332950 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/core@master] [WIP] Add InterruptMutexManager interface

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

Code is written, needs code review -- the gerrit patches are in the task description. @tstarling Is someone from your team able to take that on?

The use case for this is to allow Edit Stashing to use PoolCounter, instead of database locks (which means that we could do stashing in the secondary data center). We're open to solving this in another way instead.

Change 332951 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/extensions/PoolCounter@master] [WIP] Implement InterruptMutexManager methods in PoolCounter_Client

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

Change 532869 had a related patch set uploaded (by Krinkle; owner: Aaron Schulz):
[mediawiki/core@master] lockmanager: rewrite MySqlLockManager to use GET_LOCK() and friends

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

Just flying by here to say that having this abstracted and standardised interface for a dc-local non-blocking mutex lock manager, would also help with reducing use of the internal getLocalClusterInstance object. Apart from one or two core use cases, use of that are mostly for wanting a cheap lock that works both by default in stock MW and at WMF. By having this service class, we'd be able to transition more use away from the internal getLocalClusterInstance and to something with a proper service interface. (The alternative to using getLocalClusterInstance today, are basically to either hardcode against DB master locks for stock MW and for WMF; or to use WMF-only poolcounter and otherwise be unprotected by default; hence getLocalClusterInstance is currently a common thing to reach for).

See also T243233.

Random observation because this poped up on my radar again: I'd like to reiterate that the word "Interrupt" in the name sends me down the wrong path of thought every time. Why not just "MutexManager" or "MutexLockManager"?

Change 332950 abandoned by Aaron Schulz:

[mediawiki/core@master] [WIP] Add InterruptMutexManager interface

Reason:

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

Change 332951 abandoned by Aaron Schulz:

[mediawiki/extensions/PoolCounter@master] [WIP] Implement InterruptMutexManager methods in PoolCounter_Client

Reason:

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

Change 532869 abandoned by Aaron Schulz:

[mediawiki/core@master] lockmanager: rewrite MySqlLockManager to use GET_LOCK() and friends

Reason:

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

Change 532869 restored by Aaron Schulz:

[mediawiki/core@master] lockmanager: rewrite MySqlLockManager to use GET_LOCK() and friends

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

Change 532869 abandoned by Aaron Schulz:

[mediawiki/core@master] lockmanager: rewrite MySqlLockManager to use GET_LOCK() and friends

Reason:

PoC; going with removal

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