Page MenuHomePhabricator

When we fail to acquire a repository lock, try to provide a hint about why
ClosedPublic

Authored by epriestley on Sep 23 2018, 6:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 7:44 PM
Unknown Object (File)
Mon, Nov 18, 10:55 AM
Unknown Object (File)
Mon, Nov 18, 3:17 AM
Unknown Object (File)
Wed, Nov 13, 11:38 PM
Unknown Object (File)
Mon, Nov 11, 1:36 PM
Unknown Object (File)
Sun, Nov 10, 12:09 PM
Unknown Object (File)
Sun, Nov 10, 12:52 AM
Unknown Object (File)
Thu, Nov 7, 6:07 PM
Subscribers
None

Details

Summary

Ref T13202. See PHI889. If the lock log is enabled, we can try to offer more details about lock holders.

When we fail to acquire a lock:

  • check for recent acquisitions and suggest that this is a bottleneck issue;
  • if there are no recent acquisitions, check for the last acquisition and print details about it (what process, how long ago, whether or not we believe it was released).
Test Plan
  • Enabled the lock log.
  • Changed the lock wait time to 1 second.
  • Added a sleep(10) after grabbing the lock.
  • In one window, ran a Conduit call or a git fetch.
  • In another window, ran another operation.
  • Got useful/sensible errors for both ssh and web lock holders, for example:

PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=12609, host=orbital-3.local, sapi=apache2handler, controller=PhabricatorConduitAPIController, method=diffusion.rawdiffquery) 3 second(s) ago. There is no record of this lock being released.)

PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=65251, host=orbital-3.local, sapi=cli, argv=/Users/epriestley/dev/core/lib/phabricator/bin/ssh-exec --phabricator-ssh-device local.phacility.net --phabricator-ssh-key 2) 2 second(s) ago. There is no record of this lock being released.)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/infrastructure/util/PhabricatorGlobalLock.php
301

"processe(s)"

This revision is now accepted and ready to land.Sep 24 2018, 5:27 PM
epriestley added inline comments.
src/infrastructure/util/PhabricatorGlobalLock.php
301

This is currently intended by convention -- "processe(s)" is proto-English for translation, not an actual English word, and the text in pht() is "proto-English", not English.

The number of processes may reasonably be 1, so this should (in English) be translated as "another process" (if N = 1) or "3 other processes" (if N != 1) depending on the value.

We could write this kind of thing in different proto-English if you have a suggestion that reads more naturally.

I'm not entirely consistent about this because $limit could possibly be 1 but I didn't write "%s other processe(s)" above. This is because it seems very unlikely that $limit will ever be 1, since the program would be silly if it was. Still, providing the proto-English hint to translators consistently might be helpful, and possibly I should be more consistent about this.

I also somewhat lazily didn't actually provide English translations here. My rationale is "this is a technical area of the codebase and the proto-English is sufficiently clear" plus "probably some day we'll have a nice translation tool which will make translating this stuff easier" plus "I sure am lazy". I could less-lazily provide translations, or maybe build a very simple version of the tool as a stopgap.

src/infrastructure/util/PhabricatorGlobalLock.php
301

Haha that's fine; I just assumed you meant to write "process(es)".

Oh. Yeah, process(es) is probably better.

  • Cleaner and more consistent proto-English.
This revision was automatically updated to reflect the committed changes.