Page MenuHomePhabricator

`differential.createcomment` can race automatic revision closure
Closed, ResolvedPublic

Description

An install is seeing an issue where Phabricator's close-on-commit logic races external Conduit logic (via differental.createcomment) and both update the revision simultaneously. The revision state change from close-on-commit ends up being overwritten. Specifically, this sequence occurs:

  • The worker fires close-on-commit.
  • At around the same time, an external script fires differential.createcomment.
  • The close-on-commit applies first.
  • The differential.createcomment appears to undo the state change.

This is probably a slightly broader issue applying approximately to all edits on all objects.

The first-order fix is approximately to make shouldReadLock() return true by default (or possibly remove it entirely, so it is always-on).

A second-order fix may be to try to walk that back so that some subset of changes do not need read locks, but there's probably very little actual value in doing this: we don't expect writes to be exceptionally cheap and doing read locking should not generally change the overall cadence of edits.

Conpherence uses read locking already, so it is unlikely that enabling it has weird or far-reaching consequences.

Revisions and Commits

Event Timeline

chad renamed this task from `differential.createcomment` can race automatic revision closure to differential.createcomment can race automatic revision closure.May 15 2017, 5:38 PM
chad set the point value for this task to .
chad added a subscriber: chad.

uuuuuuuuuuuuuugh

chad renamed this task from differential.createcomment can race automatic revision closure to `differential.createcomment` can race automatic revision closure.May 15 2017, 6:27 PM