Patchwork [3,of,3] commandserver: backport handling of forking server from chgserver

login
register
mail settings
Submitter Yuya Nishihara
Date June 30, 2016, 2:26 p.m.
Message ID <0afc4036f17326996838.1467296815@mimosa>
Download mbox | patch
Permalink /patch/15685/
State Accepted
Headers show

Comments

Yuya Nishihara - June 30, 2016, 2:26 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1463811801 -32400
#      Sat May 21 15:23:21 2016 +0900
# Node ID 0afc4036f1732699683814cb2a90376e7bccae2a
# Parent  b00830fdf008c83d361b05491b023778275dfd61
commandserver: backport handling of forking server from chgserver

This is common between chg and vanilla forking server, so move it to
commandserver and unify handle().

It would be debatable whether we really need gc.collect() or not, but that
is beyond the scope of this series. Maybe we can remove gc.collect() once
all resource deallocations are switched to context manager.
Jun Wu - July 4, 2016, 5:43 p.m.
This series looks good to me. Looking forward to next steps.

Note: Sorry for the delay. I was planning to review this last week but got
addicted to improve my email tooling [1] (and it's a bit interesting now).
Finally I have the one-key-to-apply-all-patches-in-current-thread feature,
which should help me with reviewing in the future :)

[1]: https://github.com/quark-zju/sup

Excerpts from Yuya Nishihara's message of 2016-06-30 23:26:55 +0900:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1463811801 -32400
> #      Sat May 21 15:23:21 2016 +0900
> # Node ID 0afc4036f1732699683814cb2a90376e7bccae2a
> # Parent  b00830fdf008c83d361b05491b023778275dfd61
> commandserver: backport handling of forking server from chgserver
> 
> This is common between chg and vanilla forking server, so move it to
> commandserver and unify handle().
> 
> It would be debatable whether we really need gc.collect() or not, but that
> is beyond the scope of this series. Maybe we can remove gc.collect() once
> all resource deallocations are switched to context manager.
> 
> diff --git a/hgext/chgserver.py b/hgext/chgserver.py
> --- a/hgext/chgserver.py
> +++ b/hgext/chgserver.py
> @@ -41,18 +41,15 @@ Config
>  from __future__ import absolute_import
>  
>  import errno
> -import gc
>  import hashlib
>  import inspect
>  import os
> -import random
>  import re
>  import signal
>  import struct
>  import sys
>  import threading
>  import time
> -import traceback
>  
>  from mercurial.i18n import _
>  
> @@ -531,46 +528,7 @@ class chgcmdserver(commandserver.server)
>                           'setenv': setenv,
>                           'setumask': setumask})
>  
> -# 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)
> -        # change random state otherwise forked request handlers would have a
> -        # same state inherited from parent.
> -        random.seed()
> -        ui = self.server.ui
> -        sv = None
> -        try:
> -            sv = self._createcmdserver()
> -            try:
> -                sv.serve()
> -            # handle exceptions that may be raised by command server. most of
> -            # known exceptions are caught by dispatch.
> -            except error.Abort as inst:
> -                ui.warn(_('abort: %s\n') % inst)
> -            except IOError as inst:
> -                if inst.errno != errno.EPIPE:
> -                    raise
> -            except KeyboardInterrupt:
> -                pass
> -            finally:
> -                sv.cleanup()
> -        except: # re-raises
> -            # also write traceback to error channel. otherwise client cannot
> -            # see it because it is written to server's stderr by default.
> -            if sv:
> -                cerr = sv.cerr
> -            else:
> -                cerr = commandserver.channeledoutput(self.wfile, 'e')
> -            traceback.print_exc(file=cerr)
> -            raise
> -        finally:
> -            # trigger __del__ since ForkingMixIn uses os._exit
> -            gc.collect()
> -
> +class _requesthandler(commandserver._requesthandler):
>      def _createcmdserver(self):
>          ui = self.server.ui
>          repo = self.server.repo
> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -8,7 +8,9 @@
>  from __future__ import absolute_import
>  
>  import errno
> +import gc
>  import os
> +import random
>  import struct
>  import sys
>  import traceback
> @@ -338,6 +340,13 @@ class pipeservice(object):
>  
>  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)
> +        # change random state otherwise forked request handlers would have a
> +        # same state inherited from parent.
> +        random.seed()
>          ui = self.server.ui
>          sv = None
>          try:
> @@ -364,6 +373,9 @@ class _requesthandler(socketserver.Strea
>                  cerr = channeledoutput(self.wfile, 'e')
>              traceback.print_exc(file=cerr)
>              raise
> +        finally:
> +            # trigger __del__ since ForkingMixIn uses os._exit
> +            gc.collect()
>  
>      def _createcmdserver(self):
>          ui = self.server.ui
Augie Fackler - July 12, 2016, 4:51 p.m.
On Mon, Jul 04, 2016 at 06:43:04PM +0100, Jun Wu wrote:
> This series looks good to me. Looking forward to next steps.

Queued per this review and my own quick check. Thanks.

>
> Note: Sorry for the delay. I was planning to review this last week but got
> addicted to improve my email tooling [1] (and it's a bit interesting now).
> Finally I have the one-key-to-apply-all-patches-in-current-thread feature,
> which should help me with reviewing in the future :)
>
> [1]: https://github.com/quark-zju/sup
>
> Excerpts from Yuya Nishihara's message of 2016-06-30 23:26:55 +0900:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1463811801 -32400
> > #      Sat May 21 15:23:21 2016 +0900
> > # Node ID 0afc4036f1732699683814cb2a90376e7bccae2a
> > # Parent  b00830fdf008c83d361b05491b023778275dfd61
> > commandserver: backport handling of forking server from chgserver
> >
> > This is common between chg and vanilla forking server, so move it to
> > commandserver and unify handle().
> >
> > It would be debatable whether we really need gc.collect() or not, but that
> > is beyond the scope of this series. Maybe we can remove gc.collect() once
> > all resource deallocations are switched to context manager.
> >
> > diff --git a/hgext/chgserver.py b/hgext/chgserver.py
> > --- a/hgext/chgserver.py
> > +++ b/hgext/chgserver.py
> > @@ -41,18 +41,15 @@ Config
> >  from __future__ import absolute_import
> >
> >  import errno
> > -import gc
> >  import hashlib
> >  import inspect
> >  import os
> > -import random
> >  import re
> >  import signal
> >  import struct
> >  import sys
> >  import threading
> >  import time
> > -import traceback
> >
> >  from mercurial.i18n import _
> >
> > @@ -531,46 +528,7 @@ class chgcmdserver(commandserver.server)
> >                           'setenv': setenv,
> >                           'setumask': setumask})
> >
> > -# 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)
> > -        # change random state otherwise forked request handlers would have a
> > -        # same state inherited from parent.
> > -        random.seed()
> > -        ui = self.server.ui
> > -        sv = None
> > -        try:
> > -            sv = self._createcmdserver()
> > -            try:
> > -                sv.serve()
> > -            # handle exceptions that may be raised by command server. most of
> > -            # known exceptions are caught by dispatch.
> > -            except error.Abort as inst:
> > -                ui.warn(_('abort: %s\n') % inst)
> > -            except IOError as inst:
> > -                if inst.errno != errno.EPIPE:
> > -                    raise
> > -            except KeyboardInterrupt:
> > -                pass
> > -            finally:
> > -                sv.cleanup()
> > -        except: # re-raises
> > -            # also write traceback to error channel. otherwise client cannot
> > -            # see it because it is written to server's stderr by default.
> > -            if sv:
> > -                cerr = sv.cerr
> > -            else:
> > -                cerr = commandserver.channeledoutput(self.wfile, 'e')
> > -            traceback.print_exc(file=cerr)
> > -            raise
> > -        finally:
> > -            # trigger __del__ since ForkingMixIn uses os._exit
> > -            gc.collect()
> > -
> > +class _requesthandler(commandserver._requesthandler):
> >      def _createcmdserver(self):
> >          ui = self.server.ui
> >          repo = self.server.repo
> > diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> > --- a/mercurial/commandserver.py
> > +++ b/mercurial/commandserver.py
> > @@ -8,7 +8,9 @@
> >  from __future__ import absolute_import
> >
> >  import errno
> > +import gc
> >  import os
> > +import random
> >  import struct
> >  import sys
> >  import traceback
> > @@ -338,6 +340,13 @@ class pipeservice(object):
> >
> >  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)
> > +        # change random state otherwise forked request handlers would have a
> > +        # same state inherited from parent.
> > +        random.seed()
> >          ui = self.server.ui
> >          sv = None
> >          try:
> > @@ -364,6 +373,9 @@ class _requesthandler(socketserver.Strea
> >                  cerr = channeledoutput(self.wfile, 'e')
> >              traceback.print_exc(file=cerr)
> >              raise
> > +        finally:
> > +            # trigger __del__ since ForkingMixIn uses os._exit
> > +            gc.collect()
> >
> >      def _createcmdserver(self):
> >          ui = self.server.ui
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/chgserver.py b/hgext/chgserver.py
--- a/hgext/chgserver.py
+++ b/hgext/chgserver.py
@@ -41,18 +41,15 @@  Config
 from __future__ import absolute_import
 
 import errno
-import gc
 import hashlib
 import inspect
 import os
-import random
 import re
 import signal
 import struct
 import sys
 import threading
 import time
-import traceback
 
 from mercurial.i18n import _
 
@@ -531,46 +528,7 @@  class chgcmdserver(commandserver.server)
                          'setenv': setenv,
                          'setumask': setumask})
 
-# 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)
-        # change random state otherwise forked request handlers would have a
-        # same state inherited from parent.
-        random.seed()
-        ui = self.server.ui
-        sv = None
-        try:
-            sv = self._createcmdserver()
-            try:
-                sv.serve()
-            # handle exceptions that may be raised by command server. most of
-            # known exceptions are caught by dispatch.
-            except error.Abort as inst:
-                ui.warn(_('abort: %s\n') % inst)
-            except IOError as inst:
-                if inst.errno != errno.EPIPE:
-                    raise
-            except KeyboardInterrupt:
-                pass
-            finally:
-                sv.cleanup()
-        except: # re-raises
-            # also write traceback to error channel. otherwise client cannot
-            # see it because it is written to server's stderr by default.
-            if sv:
-                cerr = sv.cerr
-            else:
-                cerr = commandserver.channeledoutput(self.wfile, 'e')
-            traceback.print_exc(file=cerr)
-            raise
-        finally:
-            # trigger __del__ since ForkingMixIn uses os._exit
-            gc.collect()
-
+class _requesthandler(commandserver._requesthandler):
     def _createcmdserver(self):
         ui = self.server.ui
         repo = self.server.repo
diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -8,7 +8,9 @@ 
 from __future__ import absolute_import
 
 import errno
+import gc
 import os
+import random
 import struct
 import sys
 import traceback
@@ -338,6 +340,13 @@  class pipeservice(object):
 
 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)
+        # change random state otherwise forked request handlers would have a
+        # same state inherited from parent.
+        random.seed()
         ui = self.server.ui
         sv = None
         try:
@@ -364,6 +373,9 @@  class _requesthandler(socketserver.Strea
                 cerr = channeledoutput(self.wfile, 'e')
             traceback.print_exc(file=cerr)
             raise
+        finally:
+            # trigger __del__ since ForkingMixIn uses os._exit
+            gc.collect()
 
     def _createcmdserver(self):
         ui = self.server.ui