HomePhabricator

When applying transactions, acquire a read lock sooner

Description

When applying transactions, acquire a read lock sooner

Summary:
Depends on D20461. Ref T13276. Ref T13054.

Currently, we acquire the transaction read lock after populating "old values" in transactions and filtering transactions with no effect.

This isn't early enough to prevent all weird chaotic races: if two processes try to apply a "close revision" transaction at the same time, this can happen:

PROCESS A             PROCESS B
Old Value = Open      Old Value = Open
Transaction OK: Yes   Transaction OK: Yes
Acquire Read Lock     Acquire Read Lock
Got Read Lock!        Wait...
Apply Transactions    Wait...
New Value = Closed    Wait...
Release Lock          Wait...
                      Got Read Lock!
                      Apply Transactions
                      New Value = Closed
                      Release Lock

That's not great: both processes apply an "Open -> Closed" transaction since this was a valid transaction from the viewpoint of each process when it did the checks.

We actually want this:

PROCESS A             PROCESS B
Acquire Read Lock     Acquire Read Lock
Got Read Lock!        Wait...
Old Value = Open      Wait...
Transaction OK: Yes   Wait...
Apply Transactions    Wait...
New Value = Closed    Wait...
Release Lock          Wait...
                      Got Read Lock!
                      >>> Old Value = Closed
                      >>> Transaction Has No Effect!
                      >>> Do Nothing / Abort
                      Release Lock

Move the "lock" part up so we do that.

This may cause some kind of weird second-order effects, but T13054 went through pretty cleanly and we have to do this to get correct behavior, so we can survive those if/when they arise.

Test Plan:

  • Added a sleep(10) before the lock.
  • Ran bin/repository message --reparse X in two console windows, where X is a commit that closes revision Y and Y is open.
    • Before patch: both windows closed the revision and added duplicate transactions.
    • After patch: only one of the processes had an effect.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jmeador

Maniphest Tasks: T13276, T13054

Differential Revision: https://secure.phabricator.com/D20462