Patchwork [8,of,8,V5] worker: stop using a separate thread waiting for children

login
register
mail settings
Submitter Jun Wu
Date Nov. 15, 2016, 2:39 a.m.
Message ID <977775ff43115c001579.1479177551@x1c>
Download mbox | patch
Permalink /patch/17582/
State Accepted
Headers show

Comments

Jun Wu - Nov. 15, 2016, 2:39 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1478919967 0
#      Sat Nov 12 03:06:07 2016 +0000
# Node ID 977775ff43115c001579973ef09581f102b1b842
# Parent  3a463b68088ed4721b0b0a33504143b7eff65ade
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 977775ff4311
worker: stop using a separate thread waiting for children

Now that we have a SIGCHLD hander, and it could get executed when waiting
for I/O. It's no longer necessary to have a separated waitpid thread. So
just remove it.
via Mercurial-devel - Nov. 16, 2016, 7:17 p.m.
On Mon, Nov 14, 2016 at 6:39 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1478919967 0
> #      Sat Nov 12 03:06:07 2016 +0000
> # Node ID 977775ff43115c001579973ef09581f102b1b842
> # Parent  3a463b68088ed4721b0b0a33504143b7eff65ade
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 977775ff4311
> worker: stop using a separate thread waiting for children

Just to make sure you didn't miss 9955fc5ee24b (worker: handle worker
failures more aggressively, 2013-02-20), which has this in the
message:

"We now wait for worker processes in a separate thread, so that we can
spot failures in a timely way, wihout waiting for the progress pipe
to drain."

I haven't followed the discussion on this series, so I'm sorry if this
has already been brought up. I also don't know much about how
worker.py works, I just happened to see that commit message and
thought it sounded like you're undoing that and wanted to make sure
you had seen that commit. I have no opinion about it myself; if you
tell me it's not an issue, I will happily take your word for it.


>
> Now that we have a SIGCHLD hander, and it could get executed when waiting
> for I/O. It's no longer necessary to have a separated waitpid thread. So
> just remove it.
>
> diff --git a/mercurial/worker.py b/mercurial/worker.py
> --- a/mercurial/worker.py
> +++ b/mercurial/worker.py
> @@ -13,5 +13,4 @@ import os
>  import signal
>  import sys
> -import threading
>
>  from .i18n import _
> @@ -144,9 +143,7 @@ def _posixworker(ui, func, staticargs, a
>      os.close(wfd)
>      fp = os.fdopen(rfd, 'rb', 0)
> -    t = threading.Thread(target=waitforworkers)
> -    t.start()
>      def cleanup():
>          signal.signal(signal.SIGINT, oldhandler)
> -        t.join()
> +        waitforworkers()
>          signal.signal(signal.SIGCHLD, oldchldhandler)
>          status = problem[0]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - Nov. 16, 2016, 7:49 p.m.
Excerpts from Martin von Zweigbergk's message of 2016-11-16 11:17:03 -0800:
> On Mon, Nov 14, 2016 at 6:39 PM, Jun Wu <quark@fb.com> wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1478919967 0
> > #      Sat Nov 12 03:06:07 2016 +0000
> > # Node ID 977775ff43115c001579973ef09581f102b1b842
> > # Parent  3a463b68088ed4721b0b0a33504143b7eff65ade
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 977775ff4311
> > worker: stop using a separate thread waiting for children
> 
> Just to make sure you didn't miss 9955fc5ee24b (worker: handle worker
> failures more aggressively, 2013-02-20), which has this in the
> message:
> 
> "We now wait for worker processes in a separate thread, so that we can
> spot failures in a timely way, wihout waiting for the progress pipe
> to drain."

That is not an issue - We get "timely" notification (SIGCHLD signal) when a
child crashes when the parent is "waiting for the progress pipe to drain",
and the SIGCHLD handler will kill the workers immediately.

Beware that without "[PATCH V3] util: improve iterfile so it chooses code
path wisely", Python < 2.7.4 could be broken.

This series should be a net win. The old code could have serious issues when
other extensions spawns child processes on their own.

> I haven't followed the discussion on this series, so I'm sorry if this
> has already been brought up. I also don't know much about how
> worker.py works, I just happened to see that commit message and
> thought it sounded like you're undoing that and wanted to make sure
> you had seen that commit. I have no opinion about it myself; if you
> tell me it's not an issue, I will happily take your word for it.
> 
> >
> > Now that we have a SIGCHLD hander, and it could get executed when waiting
> > for I/O. It's no longer necessary to have a separated waitpid thread. So
> > just remove it.
> >
> > diff --git a/mercurial/worker.py b/mercurial/worker.py
> > --- a/mercurial/worker.py
> > +++ b/mercurial/worker.py
> > @@ -13,5 +13,4 @@ import os
> >  import signal
> >  import sys
> > -import threading
> >
> >  from .i18n import _
> > @@ -144,9 +143,7 @@ def _posixworker(ui, func, staticargs, a
> >      os.close(wfd)
> >      fp = os.fdopen(rfd, 'rb', 0)
> > -    t = threading.Thread(target=waitforworkers)
> > -    t.start()
> >      def cleanup():
> >          signal.signal(signal.SIGINT, oldhandler)
> > -        t.join()
> > +        waitforworkers()
> >          signal.signal(signal.SIGCHLD, oldchldhandler)
> >          status = problem[0]
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/worker.py b/mercurial/worker.py
--- a/mercurial/worker.py
+++ b/mercurial/worker.py
@@ -13,5 +13,4 @@  import os
 import signal
 import sys
-import threading
 
 from .i18n import _
@@ -144,9 +143,7 @@  def _posixworker(ui, func, staticargs, a
     os.close(wfd)
     fp = os.fdopen(rfd, 'rb', 0)
-    t = threading.Thread(target=waitforworkers)
-    t.start()
     def cleanup():
         signal.signal(signal.SIGINT, oldhandler)
-        t.join()
+        waitforworkers()
         signal.signal(signal.SIGCHLD, oldchldhandler)
         status = problem[0]