Patchwork [11,of,11] merge: don't fiddle with name lookups or i18n in hot loops

login
register
mail settings
Submitter Idan Kamara
Date Feb. 10, 2013, 11:25 a.m.
Message ID <CAMz0A7mGaLsJGuor6Swfrv4ms=7nD+o0uE6=gFivPvRCs8PtnA@mail.gmail.com>
Download mbox | patch
Permalink /patch/924/
State Superseded
Commit d9ff580fcaa26199ab8d76f35f81169431ce84b3
Headers show

Comments

Idan Kamara - Feb. 10, 2013, 11:25 a.m.
On Sun, Feb 10, 2013 at 1:08 PM, Augie Fackler <raf@durin42.com> wrote:
>
>
> On Feb 10, 2013, at 8:09 AM, Idan Kamara <idankk86@gmail.com> wrote:
>
> > On Sun, Feb 10, 2013 at 2:28 AM, Bryan O'Sullivan <bos@serpentine.com>
> > wrote:
> >> Patches are in crew now.
> >
> > Someone takes the time to look over your work (on a Saturday..),
> > raises some unsettled concerns, but you decide to push it anyway?
> >
> > Not cool.
>
> Relax - I reviewed this in person for Bryan, and he addressed both your
> and my comments before pushing.

No, he didn't. And had he given me the chance to reply to his last
response, I would have shown it:


Which is clearly wrong since the child should exit on
all exceptions.

And the only reason the sys.exit I had a problem with is
'working' is thanks to this:

http://selenic.com/repo/hg/file/0027a5cec9d0/mercurial/dispatch.py#l201
Bryan O'Sullivan - Feb. 12, 2013, 1:02 a.m.
On Sun, Feb 10, 2013 at 3:25 AM, Idan Kamara <idankk86@gmail.com> wrote:

> No, he didn't. And had he given me the chance to reply to his last
> response, I would have shown it:
>

Yep, that's a bug. But it's just a bug - I fixed another one in the new
parallel code just a little while ago.


> And the only reason the sys.exit I had a problem with is
> 'working' is thanks to this:
>
> http://selenic.com/repo/hg/file/0027a5cec9d0/mercurial/dispatch.py#l201
>

Correct, and that's deliberate. Perhaps you could tell me what you'd rather
see, because I obviously think the use of sys.exit in this context is
reasonable based on what I currently know.

Patch

diff --git a/mercurial/worker.py b/mercurial/worker.py
--- a/mercurial/worker.py
+++ b/mercurial/worker.py
@@ -68,7 +68,7 @@ 
     args - arguments to split into chunks, to pass to individual
     workers
     '''
-    if worthwhile(ui, costperarg, len(args)):
+    if True: #worthwhile(ui, costperarg, len(args)):
         return _platformworker(ui, func, staticargs, args)
     return func(*staticargs + (args,))

@@ -80,6 +80,7 @@ 
         if pid == 0:
             try:
                 os.close(rfd)
+                raise util.Abort('child %d raises error' % os.getpid())
                 for i, item in func(*(staticargs + (pargs,))):
                     os.write(wfd, '%d %s\n' % (i, item))
                 os._exit(0)
diff --git a/tests/test-parallel.t b/tests/test-parallel.t
new file mode 100644
--- /dev/null
+++ b/tests/test-parallel.t
@@ -0,0 +1,12 @@ 
+  $ hg init
+  $ echo a >> a
+  $ hg ci -qAm.
+  $ cat <<EOF | python
+  > import os, sys
+  > print 'master is', os.getpid()
+  > sys.stdout.flush()
+  > from mercurial import dispatch
+  > req = dispatch.request(['update', '0', '-q'])
+  > dispatch.dispatch(req)
+  > print 'hi from', os.getpid()
+  > EOF

Running this prints:

--- /home/idan/dev/hg/default/tests/test-parallel.t
+++ /home/idan/dev/hg/default/tests/test-parallel.t.err
@@ -10,3 +10,21 @@ 
   > dispatch.dispatch(req)
   > print 'hi from', os.getpid()
   > EOF
+  master is 1441
+  abort: child 1442 raises error
+  abort: child 1443 raises error
+  abort: child 1444 raises error
+  abort: child 1445 raises error
+  abort: child 1446 raises error
+  abort: child 1447 raises error
+  abort: child 1448 raises error
+  abort: child 1449 raises error
+  hi from 1446
+  hi from 1443
+  hi from 1444
+  hi from 1445
+  hi from 1448
+  hi from 1447
+  hi from 1449
+  hi from 1442
+  hi from 1441