Page MenuHomePhabricator

When applying transactions, acquire a read lock sooner
ClosedPublic

Authored by epriestley on Apr 22 2019, 10:26 PM.

Details

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.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Apr 22 2019, 10:26 PM
epriestley requested review of this revision.Apr 22 2019, 10:27 PM
epriestley added inline comments.Apr 22 2019, 10:31 PM
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
1012–1013

(Maybe the indent code should render an indent change if a line is blank and the adjacent lines both have an indentation marker.)

amckinley accepted this revision.Apr 23 2019, 8:03 PM
This revision is now accepted and ready to land.Apr 23 2019, 8:03 PM
jmeador added inline comments.
src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
1012–1013

Would it be too farfetched to consider collapsing indentation changes if the indentation delta is exactly the same throughout the entire section of collapsed code?

I think that's reasonable -- if there are at least, say, 11 consecutive lines affected only by the same indentation level change (or the line is blank), we could show the first 3, fold the middle 5+, then show the last 3.

Naively, I'd expect these sort of changes aren't terribly common, but maybe common enough.

This revision was automatically updated to reflect the committed changes.