Patchwork [6,of,7,V3] worker: test and set problem[0] atomically

login
register
mail settings
Submitter Jun Wu
Date Aug. 4, 2016, 6:29 p.m.
Message ID <d08d72f5d8bf302dd123.1470335347@x1c>
Download mbox | patch
Permalink /patch/16098/
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 1470334096 -3600
#      Thu Aug 04 19:08:16 2016 +0100
# Node ID d08d72f5d8bf302dd1231be410d0b23dc082eb66
# Parent  d179a66b0b15988687ba9d69c5b76e4464a457eb
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r d08d72f5d8bf
worker: test and set problem[0] atomically

Previously problem[0] is not tested and set atomically:

    if st and not problem[0]:
        # at this time, another SIGCHILD happens, killworkers may run twice
        problem[0] = st
        killworkers()

This patch addresses the issue by using itertools.count(). Its CPython
implementation will make it "atomic" at the Python code level, which is
enough for our use-case (signal handler).
Augie Fackler - Aug. 5, 2016, 6:22 p.m.
On Thu, Aug 04, 2016 at 07:29:07PM +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1470334096 -3600
> #      Thu Aug 04 19:08:16 2016 +0100
> # Node ID d08d72f5d8bf302dd1231be410d0b23dc082eb66
> # Parent  d179a66b0b15988687ba9d69c5b76e4464a457eb
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r d08d72f5d8bf
> worker: test and set problem[0] atomically
>
> Previously problem[0] is not tested and set atomically:
>
>     if st and not problem[0]:
>         # at this time, another SIGCHILD happens, killworkers may run twice
>         problem[0] = st
>         killworkers()
>
> This patch addresses the issue by using itertools.count(). Its CPython
> implementation will make it "atomic" at the Python code level, which is
> enough for our use-case (signal handler).

This is very clever, but:

1) pypy is already a thing
2) It's very clever

Could we just use a conventional lock of some sort instead?

>
> diff --git a/mercurial/worker.py b/mercurial/worker.py
> --- a/mercurial/worker.py
> +++ b/mercurial/worker.py
> @@ -8,6 +8,7 @@
>  from __future__ import absolute_import
>
>  import errno
> +import itertools
>  import os
>  import signal
>  import sys
> @@ -86,6 +87,7 @@ def _posixworker(ui, func, staticargs, a
>      oldhandler = signal.getsignal(signal.SIGINT)
>      signal.signal(signal.SIGINT, signal.SIG_IGN)
>      pids, problem = [], [0]
> +    problemcount = itertools.count()
>      def killworkers():
>          # if one worker bails, there's no good reason to wait for the rest
>          for p in pids:
> @@ -107,7 +109,7 @@ def _posixworker(ui, func, staticargs, a
>                  break
>              if p:
>                  st = _exitstatus(st)
> -            if st and not problem[0]:
> +            if st and next(problemcount) == 0:
>                  problem[0] = st
>                  killworkers()
>      def sigchldhandler(signum, frame):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Aug. 6, 2016, 3:09 a.m.
On Fri, 5 Aug 2016 14:22:09 -0400, Augie Fackler wrote:
> On Thu, Aug 04, 2016 at 07:29:07PM +0100, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1470334096 -3600
> > #      Thu Aug 04 19:08:16 2016 +0100
> > # Node ID d08d72f5d8bf302dd1231be410d0b23dc082eb66
> > # Parent  d179a66b0b15988687ba9d69c5b76e4464a457eb
> > # Available At https://bitbucket.org/quark-zju/hg-draft
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft -r d08d72f5d8bf
> > worker: test and set problem[0] atomically
> >
> > Previously problem[0] is not tested and set atomically:
> >
> >     if st and not problem[0]:
> >         # at this time, another SIGCHILD happens, killworkers may run twice
> >         problem[0] = st
> >         killworkers()
> >
> > This patch addresses the issue by using itertools.count(). Its CPython
> > implementation will make it "atomic" at the Python code level, which is
> > enough for our use-case (signal handler).
> 
> This is very clever, but:
> 
> 1) pypy is already a thing
> 2) It's very clever
> 
> Could we just use a conventional lock of some sort instead?

or could we make killworkers() safe for multiple calls?

  pids = set(...)
  while pids:
      try:
          p = pids.pop()
      except KeyError:
          break
      kill(p)

Anyway, the current killworkers() doesn't care for pid reuse. It could send
SIGTERM to already-waited pids.
Jun Wu - Aug. 6, 2016, 9:12 a.m.
Excerpts from Augie Fackler's message of 2016-08-05 14:22:09 -0400:
> On Thu, Aug 04, 2016 at 07:29:07PM +0100, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1470334096 -3600
> > #      Thu Aug 04 19:08:16 2016 +0100
> > # Node ID d08d72f5d8bf302dd1231be410d0b23dc082eb66
> > # Parent  d179a66b0b15988687ba9d69c5b76e4464a457eb
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r d08d72f5d8bf
> > worker: test and set problem[0] atomically
> >
> > Previously problem[0] is not tested and set atomically:
> >
> >     if st and not problem[0]:
> >         # at this time, another SIGCHILD happens, killworkers may run twice
> >         problem[0] = st
> >         killworkers()
> >
> > This patch addresses the issue by using itertools.count(). Its CPython
> > implementation will make it "atomic" at the Python code level, which is
> > enough for our use-case (signal handler).
> 
> This is very clever, but:
> 
> 1) pypy is already a thing
> 2) It's very clever
> 
> Could we just use a conventional lock of some sort instead?

The problem is what lock to use.
threading.Lock will dead lock since signal handler runs in a same thread.
Jun Wu - Aug. 6, 2016, 9:40 a.m.
Excerpts from Yuya Nishihara's message of 2016-08-06 12:09:36 +0900:
> On Fri, 5 Aug 2016 14:22:09 -0400, Augie Fackler wrote:
> > On Thu, Aug 04, 2016 at 07:29:07PM +0100, Jun Wu wrote:
> > > # HG changeset patch
> > > # User Jun Wu <quark@fb.com>
> > > # Date 1470334096 -3600
> > > #      Thu Aug 04 19:08:16 2016 +0100
> > > # Node ID d08d72f5d8bf302dd1231be410d0b23dc082eb66
> > > # Parent  d179a66b0b15988687ba9d69c5b76e4464a457eb
> > > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r d08d72f5d8bf
> > > worker: test and set problem[0] atomically
> > >
> > > Previously problem[0] is not tested and set atomically:
> > >
> > >     if st and not problem[0]:
> > >         # at this time, another SIGCHILD happens, killworkers may run twice
> > >         problem[0] = st
> > >         killworkers()
> > >
> > > This patch addresses the issue by using itertools.count(). Its CPython
> > > implementation will make it "atomic" at the Python code level, which is
> > > enough for our use-case (signal handler).
> > 
> > This is very clever, but:
> > 
> > 1) pypy is already a thing
> > 2) It's very clever
> > 
> > Could we just use a conventional lock of some sort instead?
> 
> or could we make killworkers() safe for multiple calls?
>
>   pids = set(...)
>   while pids:
>       try:
>           p = pids.pop()
>       except KeyError:
>           break
>       kill(p)

This relies on pop() being "atomic". I guess it still has issues with pypy.

> Anyway, the current killworkers() doesn't care for pid reuse. It could send
> SIGTERM to already-waited pids.
Jun Wu - Aug. 6, 2016, 11:09 a.m.
Excerpts from Augie Fackler's message of 2016-08-05 14:22:09 -0400:
> On Thu, Aug 04, 2016 at 07:29:07PM +0100, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1470334096 -3600
> > #      Thu Aug 04 19:08:16 2016 +0100
> > # Node ID d08d72f5d8bf302dd1231be410d0b23dc082eb66
> > # Parent  d179a66b0b15988687ba9d69c5b76e4464a457eb
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r d08d72f5d8bf
> > worker: test and set problem[0] atomically
> >
> > Previously problem[0] is not tested and set atomically:
> >
> >     if st and not problem[0]:
> >         # at this time, another SIGCHILD happens, killworkers may run twice
> >         problem[0] = st
> >         killworkers()
> >
> > This patch addresses the issue by using itertools.count(). Its CPython
> > implementation will make it "atomic" at the Python code level, which is
> > enough for our use-case (signal handler).
> 
> This is very clever, but:
> 
> 1) pypy is already a thing

I just got confirmation by arigato on #pypy that this also works in Pypy.
Same to "set.pop()". I will use the later so address the "send SIGTERM
to waited processes" issue and mention Pypy in commit message.

Cheers!
Pierre-Yves David - Aug. 6, 2016, 12:50 p.m.
On 08/06/2016 01:09 PM, Jun Wu wrote:
> Excerpts from Augie Fackler's message of 2016-08-05 14:22:09 -0400:
>> On Thu, Aug 04, 2016 at 07:29:07PM +0100, Jun Wu wrote:
>>> # HG changeset patch
>>> # User Jun Wu <quark@fb.com>
>>> # Date 1470334096 -3600
>>> #      Thu Aug 04 19:08:16 2016 +0100
>>> # Node ID d08d72f5d8bf302dd1231be410d0b23dc082eb66
>>> # Parent  d179a66b0b15988687ba9d69c5b76e4464a457eb
>>> # Available At https://bitbucket.org/quark-zju/hg-draft
>>> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r d08d72f5d8bf
>>> worker: test and set problem[0] atomically
>>>
>>> Previously problem[0] is not tested and set atomically:
>>>
>>>     if st and not problem[0]:
>>>         # at this time, another SIGCHILD happens, killworkers may run twice
>>>         problem[0] = st
>>>         killworkers()
>>>
>>> This patch addresses the issue by using itertools.count(). Its CPython
>>> implementation will make it "atomic" at the Python code level, which is
>>> enough for our use-case (signal handler).
>>
>> This is very clever, but:
>>
>> 1) pypy is already a thing
>
> I just got confirmation by arigato on #pypy that this also works in Pypy.
> Same to "set.pop()". I will use the later so address the "send SIGTERM
> to waited processes" issue and mention Pypy in commit message.

If we have anything smart relying on set.pop() being atomical, we should 
add an inline comment to point it out.

Patch

diff --git a/mercurial/worker.py b/mercurial/worker.py
--- a/mercurial/worker.py
+++ b/mercurial/worker.py
@@ -8,6 +8,7 @@ 
 from __future__ import absolute_import
 
 import errno
+import itertools
 import os
 import signal
 import sys
@@ -86,6 +87,7 @@  def _posixworker(ui, func, staticargs, a
     oldhandler = signal.getsignal(signal.SIGINT)
     signal.signal(signal.SIGINT, signal.SIG_IGN)
     pids, problem = [], [0]
+    problemcount = itertools.count()
     def killworkers():
         # if one worker bails, there's no good reason to wait for the rest
         for p in pids:
@@ -107,7 +109,7 @@  def _posixworker(ui, func, staticargs, a
                 break
             if p:
                 st = _exitstatus(st)
-            if st and not problem[0]:
+            if st and next(problemcount) == 0:
                 problem[0] = st
                 killworkers()
     def sigchldhandler(signum, frame):