[PATCH 3 of 3] localrepo: pass args and command running as store/write lock metadata
quark at fb.com
Tue Mar 21 13:39:51 EDT 2017
Excerpts from Ryan McElroy's message of 2017-03-21 17:18:21 +0000:
> On 3/21/17 4:40 PM, Jun Wu wrote:
> > Excerpts from Ryan McElroy's message of 2017-03-20 12:39:11 +0000:
> >> Overall, I like the functionality this series adds, but I'm less
> >> convinced on the specific implementation. Adding more stuff to the
> >> junkyard that is the "ui" class doesn't feel awesome, and I'm concerned
> >> with how it will interact with chg and a future direction of ui
> >> immutability.
> >> I have an alternate suggestion for how to achieve this visibility into
> >> what is going on in a repo:
> >> Instead of having this information inside of a lock, what if every
> >> process that started up in a repo tried to create a file in
> >> .hg/processes/ (or something) of the form "info.<pid>". Then you
> >> wouldn't need to pass this data through to the lock command, wouldn't
> >> need to plumb it through the ui class, and combining the lock info with
> >> the processes info would still be straightforward.
> >> This would allow you to touch only the dispatch logic and achieve
> >> something even cooler: you'd be able to see all of the processes running
> >> in a repo (even read processes, as long as the reader has write access
> >> to the repo's .hg/).
> >> Thoughts on this alternate approach?
> > Two questions:
> > 1. Who is responsible for garbage collecting those pid files, in case of
> > crashes?
> > 2. Linux has pid namespace, pid could conflict.
> > I think it's better to just rely on whatever the operating system provides
> > to convert pid to args.
> 1. Could be the same as abandoned lockfile cleanup (or maybe we just
> leave them as graves honoring the processes that came and died in this
> repo -- joking)
> 2. Already breaks mercurial locks badly due to abandoned lockfile
> cleanup, so I'm a little less concerned about that -- but there is a
> case that I might care about which would be a read-only process
> overwriting the lock holder's "pidfile"
With Docker becomes more popular, it's more common to have these situations
nowadays. So I think we have to deal with pid namespaces.
The lock file is not that fragile - normal Python code crash won't cause it
to be orphaned because the interpretor still executes "finally" block, it
has to be the crash of the interpretor or a forced kill.
> These points do raise the complexity and risk of my proposed approach. I
> would rather have this information *somehow* than worrying about exactly
> how, and the current approach in this series seems lower risk. So I'm
> okay going with the approach suggested in these patches (but with fixes
> for the issues raised).
More information about the Mercurial-devel