Patchwork [V2] chgserver: create new process group after fork (issue5051)

login
register
mail settings
Submitter Jun Wu
Date Jan. 20, 2016, 10:44 a.m.
Message ID <bc3a251a56a50ee825ff.1453286690@x1c>
Download mbox | patch
Permalink /patch/12841/
State Accepted
Commit 83fc0c0556644d742e3d6eecfb38e742005799cc
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Jan. 20, 2016, 10:44 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1453227613 0
#      Tue Jan 19 18:20:13 2016 +0000
# Node ID bc3a251a56a50ee825ffffcf941973ecae3d78bc
# Parent  d2c5ad3deccb5a504e2553652b66a4110db68afb
chgserver: create new process group after fork (issue5051)

This is to make SIGTSTP work. Before the patch, the server process group is
considered "orphaned" and will ignore SIGTSTP, SIGTTIN, SIGTTOU, according to
POSIX. See the comment above `will_become_orphaned_pgrp` in `kernel/exit.c`
from Linux 4.3 for details.

SIGTSTP is important if chgserver runs some ncurses commend like `commit -i`.
Ncurses has its own SIGTSTP handler which will do the following:

  1. Clean the screen
  2. Stop itself by resending SIGTSTP to itself
  3. Restore the screen

If SIGTSTP is ignored, step 2 will be a noop, which means the process cannot
be suspended properly.

In order to make things work, chg client needs to forward SIGTSTP and SIGCONT
to server as well.
Yuya Nishihara - Jan. 20, 2016, 2:32 p.m.
On Wed, 20 Jan 2016 10:44:50 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1453227613 0
> #      Tue Jan 19 18:20:13 2016 +0000
> # Node ID bc3a251a56a50ee825ffffcf941973ecae3d78bc
> # Parent  d2c5ad3deccb5a504e2553652b66a4110db68afb
> chgserver: create new process group after fork (issue5051)

LGTM, thanks for investigation.
I'll queue this when default branch is reopened.

> This is to make SIGTSTP work. Before the patch, the server process group is
> considered "orphaned" and will ignore SIGTSTP, SIGTTIN, SIGTTOU, according to
> POSIX. See the comment above `will_become_orphaned_pgrp` in `kernel/exit.c`
> from Linux 4.3 for details.

(I don't have a good knowledge of session/job control, I just read several
manpages about orphaned process group.)

Assuming the master server process (= session leader) is a job-control process,
forked workers can be considered job processes. So it seems legit to give a
separate process group to each worker.
Yuya Nishihara - Feb. 6, 2016, 11:06 a.m.
On Wed, 20 Jan 2016 23:32:50 +0900, Yuya Nishihara wrote:
> On Wed, 20 Jan 2016 10:44:50 +0000, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1453227613 0
> > #      Tue Jan 19 18:20:13 2016 +0000
> > # Node ID bc3a251a56a50ee825ffffcf941973ecae3d78bc
> > # Parent  d2c5ad3deccb5a504e2553652b66a4110db68afb
> > chgserver: create new process group after fork (issue5051)
> 
> LGTM, thanks for investigation.
> I'll queue this when default branch is reopened.

Pushed this to clowncopter.

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -354,6 +354,10 @@ 
 # copied from mercurial/commandserver.py
 class _requesthandler(SocketServer.StreamRequestHandler):
     def handle(self):
+        # use a different process group from the master process, making this
+        # process pass kernel "is_current_pgrp_orphaned" check so signals like
+        # SIGTSTP, SIGTTIN, SIGTTOU are not ignored.
+        os.setpgid(0, 0)
         ui = self.server.ui
         repo = self.server.repo
         sv = chgcmdserver(ui, repo, self.rfile, self.wfile, self.connection)