| 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
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
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.
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.
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.
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!
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):
# 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).