Patchwork [4,of,7,V3] worker: make waitforworkers reentrant

login
register
mail settings
Submitter Jun Wu
Date Aug. 4, 2016, 6:29 p.m.
Message ID <74a3dd438c5645cc1a68.1470335345@x1c>
Download mbox | patch
Permalink /patch/16095/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Aug. 4, 2016, 6:29 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1469737019 -3600
#      Thu Jul 28 21:16:59 2016 +0100
# Node ID 74a3dd438c5645cc1a685e788d86e70698bfe6c9
# Parent  521d7fac970d711766e61b7b54d20ddefc4be406
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 74a3dd438c56
worker: make waitforworkers reentrant

We are going to use it in SIGCHLD handler. The handler will be executed in the
main thread with the non-blocking version of waitpid, while the waitforworkers
thread runs the blocking version. It's possible that one of them collects a
worker and makes the other error out. This patch handles these error: ECHILD
is ignored. EINTR needs a retry.
Augie Fackler - Aug. 5, 2016, 6:20 p.m.
On Thu, Aug 04, 2016 at 07:29:05PM +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1469737019 -3600
> #      Thu Jul 28 21:16:59 2016 +0100
> # Node ID 74a3dd438c5645cc1a685e788d86e70698bfe6c9
> # Parent  521d7fac970d711766e61b7b54d20ddefc4be406
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 74a3dd438c56
> worker: make waitforworkers reentrant
>
> We are going to use it in SIGCHLD handler. The handler will be executed in the
> main thread with the non-blocking version of waitpid, while the waitforworkers
> thread runs the blocking version. It's possible that one of them collects a
> worker and makes the other error out. This patch handles these error: ECHILD
> is ignored. EINTR needs a retry.
>
> diff --git a/mercurial/worker.py b/mercurial/worker.py
> --- a/mercurial/worker.py
> +++ b/mercurial/worker.py
> @@ -96,7 +96,15 @@ def _posixworker(ui, func, staticargs, a
>                      raise
>      def waitforworkers(blocking=True):
>          for pid in pids:
> -            p, st = os.waitpid(pid, 0 if blocking else os.WNOHANG)
> +            p = st = 0
> +            while True:
> +                try:
> +                    p, st = os.waitpid(pid, 0 if blocking else os.WNOHANG)
> +                except OSError as e:
> +                    if e.errno == errno.EINTR:
> +                        continue
> +                    # otherwise it's ECHILD and is ignored

shouldn't we explicitly look for ECHILD and ignore that? EINVAL could
also happen if the arguments to os.waitpid are invalid, which could
lead to some very confusing debugging sessions?

> +                break
>              if p:
>                  st = _exitstatus(st)
>              if st and not problem[0]:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - Aug. 6, 2016, 9:16 a.m.
Excerpts from Augie Fackler's message of 2016-08-05 14:20:15 -0400:
> On Thu, Aug 04, 2016 at 07:29:05PM +0100, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1469737019 -3600
> > #      Thu Jul 28 21:16:59 2016 +0100
> > # Node ID 74a3dd438c5645cc1a685e788d86e70698bfe6c9
> > # Parent  521d7fac970d711766e61b7b54d20ddefc4be406
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 74a3dd438c56
> > worker: make waitforworkers reentrant
> >
> > We are going to use it in SIGCHLD handler. The handler will be executed in the
> > main thread with the non-blocking version of waitpid, while the waitforworkers
> > thread runs the blocking version. It's possible that one of them collects a
> > worker and makes the other error out. This patch handles these error: ECHILD
> > is ignored. EINTR needs a retry.
> >
> > diff --git a/mercurial/worker.py b/mercurial/worker.py
> > --- a/mercurial/worker.py
> > +++ b/mercurial/worker.py
> > @@ -96,7 +96,15 @@ def _posixworker(ui, func, staticargs, a
> >                      raise
> >      def waitforworkers(blocking=True):
> >          for pid in pids:
> > -            p, st = os.waitpid(pid, 0 if blocking else os.WNOHANG)
> > +            p = st = 0
> > +            while True:
> > +                try:
> > +                    p, st = os.waitpid(pid, 0 if blocking else os.WNOHANG)
> > +                except OSError as e:
> > +                    if e.errno == errno.EINTR:
> > +                        continue
> > +                    # otherwise it's ECHILD and is ignored
> 
> shouldn't we explicitly look for ECHILD and ignore that? EINVAL could
> also happen if the arguments to os.waitpid are invalid, which could
> lead to some very confusing debugging sessions?

I will add the if condition. os.WNOHANG and 0 are both valid values so
EINVAL is actually not possible.

Patch

diff --git a/mercurial/worker.py b/mercurial/worker.py
--- a/mercurial/worker.py
+++ b/mercurial/worker.py
@@ -96,7 +96,15 @@  def _posixworker(ui, func, staticargs, a
                     raise
     def waitforworkers(blocking=True):
         for pid in pids:
-            p, st = os.waitpid(pid, 0 if blocking else os.WNOHANG)
+            p = st = 0
+            while True:
+                try:
+                    p, st = os.waitpid(pid, 0 if blocking else os.WNOHANG)
+                except OSError as e:
+                    if e.errno == errno.EINTR:
+                        continue
+                    # otherwise it's ECHILD and is ignored
+                break
             if p:
                 st = _exitstatus(st)
             if st and not problem[0]: