Patchwork D7585: remotefilelog: add a developer option to wait for background processes

login
register
mail settings
Submitter phabricator
Date Dec. 9, 2019, 1:02 p.m.
Message ID <differential-rev-PHID-DREV-k3nndiyk622exsjtpmw7-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43650/
State Superseded
Headers show

Comments

phabricator - Dec. 9, 2019, 1:02 p.m.
marmoute created this revision.
marmoute added a comment.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.


  target is the stable-branch

REVISION SUMMARY
  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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7585

AFFECTED FILES
  hgext/remotefilelog/__init__.py
  hgext/remotefilelog/repack.py
  hgext/remotefilelog/shallowrepo.py

CHANGE DETAILS




To: marmoute, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 9, 2019, 3:14 p.m.
durin42 added inline comments.

INLINE COMMENTS

> shallowrepo.py:237
> +            if repo.ui.configbool(b'devel', b'remotefilelog.bg-wait'):
> +                kwargs['record_wait'] = repo.ui.atexit
> +

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.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7585/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7585

To: marmoute, #hg-reviewers
Cc: durin42, mercurial-devel

Patch

diff --git a/hgext/remotefilelog/shallowrepo.py b/hgext/remotefilelog/shallowrepo.py
--- a/hgext/remotefilelog/shallowrepo.py
+++ b/hgext/remotefilelog/shallowrepo.py
@@ -232,8 +232,12 @@ 
                 cmd += [b'-r', revs]
             # We know this command will find a binary, so don't block
             # on it starting.
+            kwargs = {}
+            if repo.ui.configbool(b'devel', b'remotefilelog.bg-wait'):
+                kwargs['record_wait'] = repo.ui.atexit
+
             procutil.runbgcommand(
-                cmd, encoding.environ, ensurestart=ensurestart
+                cmd, encoding.environ, ensurestart=ensurestart, **kwargs
             )
 
         def prefetch(self, revs, base=None, pats=None, opts=None):
diff --git a/hgext/remotefilelog/repack.py b/hgext/remotefilelog/repack.py
--- a/hgext/remotefilelog/repack.py
+++ b/hgext/remotefilelog/repack.py
@@ -48,7 +48,13 @@ 
         cmd.append(b'--packsonly')
     repo.ui.warn(msg)
     # 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):
diff --git a/hgext/remotefilelog/__init__.py b/hgext/remotefilelog/__init__.py
--- a/hgext/remotefilelog/__init__.py
+++ b/hgext/remotefilelog/__init__.py
@@ -230,6 +230,7 @@ 
 configitem(b'packs', b'maxchainlen', default=1000)
 
 configitem(b'devel', b'remotefilelog.ensurestart', default=False)
+configitem(b'devel', b'remotefilelog.bg-wait', default=False)
 
 #  default TTL limit is 30 days
 _defaultlimit = 60 * 60 * 24 * 30