Quantcast

[PATCH stateful-chg] commandserver: move _socket state to handler

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH stateful-chg] commandserver: move _socket state to handler

Jun Wu
# HG changeset patch
# User Jun Wu <[hidden email]>
# Date 1495042747 25200
#      Wed May 17 10:39:07 2017 -0700
# Node ID 28a79744e54d6b9c08d8d835c5bafdc90ba3c02d
# Parent  37bcb4665529f5cc59b8dffb1014ac0cab37492c
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 28a79744e54d
commandserver: move _socket state to handler

This patch moves "_socket" state service to servicehandler. It makes
"_socket" replace-able inside service main loop, which is needed for
stateful chg to be functional without subclassing the service class.

diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
--- a/mercurial/chgserver.py
+++ b/mercurial/chgserver.py
@@ -479,5 +479,5 @@ def _hashaddress(address, hashstr):
     return '%s-%s' % (os.path.join(dirname, basename), hashstr)
 
-class chgunixservicehandler(object):
+class chgunixservicehandler(commandserver.unixservicehandler):
     """Set of operations for chg services"""
 
@@ -485,5 +485,5 @@ class chgunixservicehandler(object):
 
     def __init__(self, ui):
-        self.ui = ui
+        super(chgunixservicehandler, self).__init__(ui)
         self._idletimeout = ui.configint('chgserver', 'idletimeout', 3600)
         self._lastactive = time.time()
diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -407,4 +407,5 @@ class unixservicehandler(object):
     def __init__(self, ui):
         self.ui = ui
+        self._socket = None
 
     def bindsocket(self, sock, address):
@@ -414,4 +415,19 @@ class unixservicehandler(object):
         self.ui.flush()  # avoid buffering of status message
 
+    def getsocket(self):
+        """Return a socket for the server to use. Create on demand.
+
+        This will be called inside service's main loop per iteration. A
+        different socket can be returned and service can use that in a new
+        iteration. Make sure the returned one is the only one opened."""
+        if self._socket is None:
+            self._socket = socket.socket(socket.AF_UNIX)
+        return self._socket
+
+    def closesocket(self):
+        if self._socket is not None:
+            self._socket.close()
+            self._socket = None
+
     def unlinksocket(self, address):
         os.unlink(address)
@@ -444,5 +460,4 @@ class unixforkingservice(object):
             raise error.Abort(_('no socket path specified with --address'))
         self._servicehandler = handler or unixservicehandler(ui)
-        self._sock = None
         self._oldsigchldhandler = None
         self._workerpids = set()  # updated by signal handler; do not iterate
@@ -450,6 +465,6 @@ class unixforkingservice(object):
 
     def init(self):
-        self._sock = socket.socket(socket.AF_UNIX)
-        self._servicehandler.bindsocket(self._sock, self.address)
+        sock = self._servicehandler.getsocket()
+        self._servicehandler.bindsocket(sock, self.address)
         o = signal.signal(signal.SIGCHLD, self._sigchldhandler)
         self._oldsigchldhandler = o
@@ -463,5 +478,5 @@ class unixforkingservice(object):
     def _cleanup(self):
         signal.signal(signal.SIGCHLD, self._oldsigchldhandler)
-        self._sock.close()
+        self._servicehandler.closesocket()
         self._unlinksocket()
         # don't kill child processes as they have active clients, just wait
@@ -486,6 +501,7 @@ class unixforkingservice(object):
                 self._unlinksocket()
                 exiting = True
+            sock = self._servicehandler.getsocket()
             try:
-                ready = select.select([self._sock], [], [], h.pollinterval)[0]
+                ready = select.select([sock], [], [], h.pollinterval)[0]
                 if not ready:
                     # only exit if we completed all queued requests
@@ -493,5 +509,5 @@ class unixforkingservice(object):
                         break
                     continue
-                conn, _addr = self._sock.accept()
+                conn, _addr = sock.accept()
             except (select.error, socket.error) as inst:
                 if inst.args[0] == errno.EINTR:
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH stateful-chg] commandserver: move _socket state to handler

Yuya Nishihara
On Wed, 17 May 2017 10:39:48 -0700, Jun Wu wrote:

> # HG changeset patch
> # User Jun Wu <[hidden email]>
> # Date 1495042747 25200
> #      Wed May 17 10:39:07 2017 -0700
> # Node ID 28a79744e54d6b9c08d8d835c5bafdc90ba3c02d
> # Parent  37bcb4665529f5cc59b8dffb1014ac0cab37492c
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 28a79744e54d
> commandserver: move _socket state to handler
>
> This patch moves "_socket" state service to servicehandler. It makes
> "_socket" replace-able inside service main loop, which is needed for
> stateful chg to be functional without subclassing the service class.

Can you give some pointers to understand how getsocket() will be used in chg?

> -class chgunixservicehandler(object):
> +class chgunixservicehandler(commandserver.unixservicehandler):

Nit: I prefer not inheriting methods that are mostly overridden.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH stateful-chg] commandserver: move _socket state to handler

Jun Wu
Excerpts from Yuya Nishihara's message of 2017-05-18 22:59:37 +0900:
> Can you give some pointers to understand how getsocket() will be used in chg?

  # service.mainloop
  while not shouldexit:
    sock = handler.getsocket()
  # handler.getsocket
  - handler reads IPC, may fork to preload in background
    - the non-forked handler returns the old sock
    - the forked handler does the following:
      - preload
      - atomic replace socket (so the old process will exit)
      - return socket (so service starts to use the new one)

> > -class chgunixservicehandler(object):
> > +class chgunixservicehandler(commandserver.unixservicehandler):
>
> Nit: I prefer not inheriting methods that are mostly overridden.

Maybe we can use some mixins.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH stateful-chg] commandserver: move _socket state to handler

Yuya Nishihara
On Thu, 18 May 2017 09:42:44 -0700, Jun Wu wrote:

> Excerpts from Yuya Nishihara's message of 2017-05-18 22:59:37 +0900:
> > Can you give some pointers to understand how getsocket() will be used in chg?
>
>   # service.mainloop
>   while not shouldexit:
>     sock = handler.getsocket()
>   # handler.getsocket
>   - handler reads IPC, may fork to preload in background
>     - the non-forked handler returns the old sock
>     - the forked handler does the following:
>       - preload
>       - atomic replace socket (so the old process will exit)
>       - return socket (so service starts to use the new one)

I recalled that, thanks. Can we move the fork logic to commandserver.py?

  # unixforkingservice
  mainloop:
    if h.need_to_reinitialize_in_background?  # reads IPC
      fork
      if child:
        reinitialize_server
      else:
        keep serving in the current process

  reinitialize_server:
    close_existing_socket
    h.preload
    h.rebind_socket(new_socket)

IMHO, it isn't easy to abstract fork() stuff cleanly. Doing that in service
subclass or handler would make it harder to track where resources are created
and released, and could lead to subtle bugs.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH stateful-chg] commandserver: move _socket state to handler

Jun Wu
Excerpts from Yuya Nishihara's message of 2017-05-20 01:01:41 +0900:

> I recalled that, thanks. Can we move the fork logic to commandserver.py?
>
>   # unixforkingservice
>   mainloop:
>     if h.need_to_reinitialize_in_background?  # reads IPC
>       fork
>       if child:
>         reinitialize_server
>       else:
>         keep serving in the current process
>
>   reinitialize_server:
>     close_existing_socket
>     h.preload
>     h.rebind_socket(new_socket)
>
> IMHO, it isn't easy to abstract fork() stuff cleanly. Doing that in service
> subclass or handler would make it harder to track where resources are created
> and released, and could lead to subtle bugs.

I think that works. So this patch could be dropped. And maybe take the
dispatch.py change so the next steps are easier.

https://bitbucket.org/quark-zju/hg-draft/commits/branch/chg-stateful is a
preview about what's next roughly.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Loading...