Patchwork worker: flush ui buffers before running the worker

login
register
mail settings
Submitter David Soria Parra
Date March 28, 2017, 5:22 p.m.
Message ID <b89199982355728a3741.1490721738@davidsp-mbp-2.local>
Download mbox | patch
Permalink /patch/19790/
State Accepted
Headers show

Comments

David Soria Parra - March 28, 2017, 5:22 p.m.
# HG changeset patch
# User David Soria Parra <davidsp@fb.com>
# Date 1490721698 25200
#      Tue Mar 28 10:21:38 2017 -0700
# Node ID b89199982355728a3741edf5518c72ccc52ee33c
# Parent  331cc4433efe0d897bb16ad4ff08a3fbe850869b
worker: flush ui buffers before running the worker

a91c6275 introduces flushing ui buffers after a worker finished. If the ui was
not flushed before the worker was started, fork will copy the existing buffers
to the worker. This causes messages issued before the worker started to be
written to the terminal for each worker.

We are now flushing the ui before we start a worker and add an appropriate test
which will fail before this patch.
Ryan McElroy - March 28, 2017, 7:03 p.m.
On 3/28/17 6:22 PM, David Soria Parra wrote:
> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1490721698 25200
> #      Tue Mar 28 10:21:38 2017 -0700
> # Node ID b89199982355728a3741edf5518c72ccc52ee33c
> # Parent  331cc4433efe0d897bb16ad4ff08a3fbe850869b
> worker: flush ui buffers before running the worker

This one makes sense to me and code looks good. Marked as pre-reviewed.

>
> a91c6275 introduces flushing ui buffers after a worker finished. If the ui was
> not flushed before the worker was started, fork will copy the existing buffers
> to the worker. This causes messages issued before the worker started to be
> written to the terminal for each worker.
>
> We are now flushing the ui before we start a worker and add an appropriate test
> which will fail before this patch.
>
> diff --git a/mercurial/worker.py b/mercurial/worker.py
> --- a/mercurial/worker.py
> +++ b/mercurial/worker.py
> @@ -133,6 +133,7 @@
>           if problem[0]:
>               killworkers()
>       oldchldhandler = signal.signal(signal.SIGCHLD, sigchldhandler)
> +    ui.flush()
>       for pargs in partition(args, workers):
>           pid = os.fork()
>           if pid == 0:
> diff --git a/tests/test-worker.t b/tests/test-worker.t
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-worker.t
> @@ -0,0 +1,52 @@
> +Test UI worker interaction
> +
> +  $ cat > t.py <<EOF
> +  > from __future__ import absolute_import, print_function
> +  > from mercurial import (
> +  >     worker, ui as uimod, cmdutil
> +  > )
> +  > def runme(ui, args):
> +  >     for arg in args:
> +  >         ui.status('run\n')
> +  >         yield 1, arg
> +  > cmdtable = {}
> +  > command = cmdutil.command(cmdtable)
> +  > @command('test', [], 'hg test [COST]')
> +  > def t(ui, repo, cost=1.0):
> +  >     cost = float(cost)
> +  >     ui.status('start\n')
> +  >     runs = worker.worker(ui, cost, runme, (ui,), range(8))
> +  >     for n, i in runs:
> +  >         pass
> +  >     ui.status('done\n')
> +  > EOF
> +  $ abspath=`pwd`/t.py
> +  $ hg init
> +
> +Run tests with worker enable by forcing a heigh cost
> +
> +  $ hg --config "extensions.t=$abspath" test 100000.0
> +  start
> +  run
> +  run
> +  run
> +  run
> +  run
> +  run
> +  run
> +  run
> +  done
> +
> +Run tests without worker by forcing a low cost
> +
> +  $ hg --config "extensions.t=$abspath" test 0.0000001
> +  start
> +  run
> +  run
> +  run
> +  run
> +  run
> +  run
> +  run
> +  run
> +  done
>
Yuya Nishihara - March 29, 2017, 2:10 p.m.
On Tue, 28 Mar 2017 10:22:18 -0700, David Soria Parra wrote:
> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1490721698 25200
> #      Tue Mar 28 10:21:38 2017 -0700
> # Node ID b89199982355728a3741edf5518c72ccc52ee33c
> # Parent  331cc4433efe0d897bb16ad4ff08a3fbe850869b
> worker: flush ui buffers before running the worker

Queued, thanks.

This should mitigate the problem reported as
https://bz.mercurial-scm.org/show_bug.cgi?id=4929

Patch

diff --git a/mercurial/worker.py b/mercurial/worker.py
--- a/mercurial/worker.py
+++ b/mercurial/worker.py
@@ -133,6 +133,7 @@ 
         if problem[0]:
             killworkers()
     oldchldhandler = signal.signal(signal.SIGCHLD, sigchldhandler)
+    ui.flush()
     for pargs in partition(args, workers):
         pid = os.fork()
         if pid == 0:
diff --git a/tests/test-worker.t b/tests/test-worker.t
new file mode 100644
--- /dev/null
+++ b/tests/test-worker.t
@@ -0,0 +1,52 @@ 
+Test UI worker interaction
+
+  $ cat > t.py <<EOF
+  > from __future__ import absolute_import, print_function
+  > from mercurial import (
+  >     worker, ui as uimod, cmdutil
+  > )
+  > def runme(ui, args):
+  >     for arg in args:
+  >         ui.status('run\n')
+  >         yield 1, arg
+  > cmdtable = {}
+  > command = cmdutil.command(cmdtable)
+  > @command('test', [], 'hg test [COST]')
+  > def t(ui, repo, cost=1.0):
+  >     cost = float(cost)
+  >     ui.status('start\n')
+  >     runs = worker.worker(ui, cost, runme, (ui,), range(8))
+  >     for n, i in runs:
+  >         pass
+  >     ui.status('done\n')
+  > EOF
+  $ abspath=`pwd`/t.py
+  $ hg init
+
+Run tests with worker enable by forcing a heigh cost
+
+  $ hg --config "extensions.t=$abspath" test 100000.0
+  start
+  run
+  run
+  run
+  run
+  run
+  run
+  run
+  run
+  done
+
+Run tests without worker by forcing a low cost
+
+  $ hg --config "extensions.t=$abspath" test 0.0000001
+  start
+  run
+  run
+  run
+  run
+  run
+  run
+  run
+  run
+  done