This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7585

remotefilelog: add a developer option to wait for background processes
ClosedPublic

Authored by marmoute on Dec 9 2019, 8:02 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG63bb6dc62f24: remotefilelog: add a developer option to wait for background processes
Summary

In order to block the main command on the subprocess exiting, we ensure the
repo's ui object will call the subprocess.wait() method to ensure the top-level
hg process doesn't exit until all background processes have also done so.

Currently, in the tests, most operation spawning background process as followed
by commands waiting for these operations to complete. However this waiting is
racy. First because it seems like we can start waiting before the background
operation actually start, in which case it is prematurely detected as "done".
Second, because some commands may spawn multiple background operation for the
same operation (eg: rebase can apparently trigger multiple prefetch). The
current approach could be updated to maybe handle the first issue, but the
second one will never be properly handled.

In most case, we do not care that the bg process keep running after the command
end. (Since we explicitly wait for them to end before doing anything else). So
we add an option to wait on the background process before exiting the command.
We'll put it in use in the next changeset.

Diff Detail

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

Event Timeline

marmoute created this revision.Dec 9 2019, 8:02 AM

target is the stable-branch

durin42 added a subscriber: durin42.Dec 9 2019, 10:14 AM
durin42 added inline comments.
hgext/remotefilelog/shallowrepo.py
237

It took me some very careful reasoning to figure this out. Could you amend the log message to include something like the following?

In order to block on the subprocess exiting, we ensure the repo's ui object will call the subprocess.wait() method to ensure the top-level hg process doesn't exit until all background processes have also done so.

Maybe it's just I didn't have tea this morning, but this cost me a pretty decent chunk of review time.

marmoute edited the summary of this revision. (Show Details)Dec 9 2019, 10:51 AM
marmoute added a commit: rHG63bb6dc62f24: remotefilelog: add a developer option to wait for background processes.Dec 11 2019, 12:28 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG63bb6dc62f24: remotefilelog: add a developer option to wait for background processes (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 18610

hgext/remotefilelog/__init__.py

configitem(b'remotefilelog', b'gcrepack', default=False) configitem(b'remotefilelog', b'gcrepack', default=False)
configitem(b'remotefilelog', b'repackonhggc', default=False) configitem(b'remotefilelog', b'repackonhggc', default=False)
configitem(b'repack', b'chainorphansbysize', default=True, experimental=True) configitem(b'repack', b'chainorphansbysize', default=True, experimental=True)
configitem(b'packs', b'maxpacksize', default=0) configitem(b'packs', b'maxpacksize', default=0)
configitem(b'packs', b'maxchainlen', default=1000) configitem(b'packs', b'maxchainlen', default=1000)
configitem(b'devel', b'remotefilelog.ensurestart', default=False) configitem(b'devel', b'remotefilelog.ensurestart', default=False)
configitem(b'devel', b'remotefilelog.bg-wait', default=False)
# default TTL limit is 30 days # default TTL limit is 30 days
_defaultlimit = 60 * 60 * 24 * 30 _defaultlimit = 60 * 60 * 24 * 30
configitem(b'remotefilelog', b'nodettl', default=_defaultlimit) configitem(b'remotefilelog', b'nodettl', default=_defaultlimit)
configitem(b'remotefilelog', b'data.gencountlimit', default=2), configitem(b'remotefilelog', b'data.gencountlimit', default=2),
configitem( configitem(
b'remotefilelog', b'data.generations', default=[b'1GB', b'100MB', b'1MB'] b'remotefilelog', b'data.generations', default=[b'1GB', b'100MB', b'1MB']

hgext/remotefilelog/repack.py

msg = _(b"(running background repack)\n") msg = _(b"(running background repack)\n")
if incremental: if incremental:
cmd.append(b'--incremental') cmd.append(b'--incremental')
msg = _(b"(running background incremental repack)\n") msg = _(b"(running background incremental repack)\n")
if packsonly: if packsonly:
cmd.append(b'--packsonly') cmd.append(b'--packsonly')
repo.ui.warn(msg) repo.ui.warn(msg)
# We know this command will find a binary, so don't block on it starting. # We know this command will find a binary, so don't block on it starting.
procutil.runbgcommand(cmd, encoding.environ, ensurestart=ensurestart) kwargs = {}
if repo.ui.configbool(b'devel', b'remotefilelog.bg-wait'):
kwargs['record_wait'] = repo.ui.atexit
procutil.runbgcommand(
cmd, encoding.environ, ensurestart=ensurestart, **kwargs
)
def fullrepack(repo, options=None): def fullrepack(repo, options=None):
"""If ``packsonly`` is True, stores creating only loose objects are skipped. """If ``packsonly`` is True, stores creating only loose objects are skipped.
""" """
if util.safehasattr(repo, 'shareddatastores'): if util.safehasattr(repo, 'shareddatastores'):
datasource = contentstore.unioncontentstore(*repo.shareddatastores) datasource = contentstore.unioncontentstore(*repo.shareddatastores)
historysource = metadatastore.unionmetadatastore( historysource = metadatastore.unionmetadatastore(

hgext/remotefilelog/shallowrepo.py

""" """
cmd = [procutil.hgexecutable(), b'-R', repo.origroot, b'prefetch'] cmd = [procutil.hgexecutable(), b'-R', repo.origroot, b'prefetch']
if repack: if repack:
cmd.append(b'--repack') cmd.append(b'--repack')
if revs: if revs:
cmd += [b'-r', revs] cmd += [b'-r', revs]
# We know this command will find a binary, so don't block # We know this command will find a binary, so don't block
# on it starting. # on it starting.
kwargs = {}
if repo.ui.configbool(b'devel', b'remotefilelog.bg-wait'):
kwargs['record_wait'] = repo.ui.atexit
durin42Unsubmitted
Not Done

It took me some very careful reasoning to figure this out. Could you amend the log message to include something like the following?

In order to block on the subprocess exiting, we ensure the repo's ui object will call the subprocess.wait() method to ensure the top-level hg process doesn't exit until all background processes have also done so.

Maybe it's just I didn't have tea this morning, but this cost me a pretty decent chunk of review time.

durin42: It took me some very careful reasoning to figure this out. Could you amend the log message to…
procutil.runbgcommand( procutil.runbgcommand(
cmd, encoding.environ, ensurestart=ensurestart cmd, encoding.environ, ensurestart=ensurestart, **kwargs
) )
def prefetch(self, revs, base=None, pats=None, opts=None): def prefetch(self, revs, base=None, pats=None, opts=None):
"""Prefetches all the necessary file revisions for the given revs """Prefetches all the necessary file revisions for the given revs
Optionally runs repack in background Optionally runs repack in background
""" """
with repo._lock( with repo._lock(
repo.svfs, repo.svfs,