Patchwork cmdserver: add command to get pid of server handling current connection

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 17, 2014, 3:39 p.m.
Message ID <1b432767e0829aba65a8.1413560340@mimosa>
Download mbox | patch
Permalink /patch/6369/
State Accepted
Headers show

Comments

Yuya Nishihara - Oct. 17, 2014, 3:39 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1413556690 -32400
#      Fri Oct 17 23:38:10 2014 +0900
# Node ID 1b432767e0829aba65a8f63bdcdb3bb9cabbce4f
# Parent  840be5ca03e1db16ba994e55597771c418166c97
cmdserver: add command to get pid of server handling current connection

Since unix-mode server forks child process per request, client cannot know
the pid of the server, which is necessary to send SIGINT for example.

Though Linux has getsockopt(SO_PEERCRED), it cannot be used because the server
fork()s after accept().  So commandserver should provide "getpid" command.
Mads Kiilerich - Oct. 17, 2014, 3:49 p.m.
On 10/17/2014 05:39 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1413556690 -32400
> #      Fri Oct 17 23:38:10 2014 +0900
> # Node ID 1b432767e0829aba65a8f63bdcdb3bb9cabbce4f
> # Parent  840be5ca03e1db16ba994e55597771c418166c97
> cmdserver: add command to get pid of server handling current connection
>
> Since unix-mode server forks child process per request, client cannot know
> the pid of the server, which is necessary to send SIGINT for example.
>
> Though Linux has getsockopt(SO_PEERCRED), it cannot be used because the server
> fork()s after accept().  So commandserver should provide "getpid" command.

Wouldn't it be better to keep it more high level and just have a 'stop', 
'die' or 'kill' command?

I can however see how this can be convenient for low level debugging, 
and highlevel stuff should probably not care about killing it all ...

/Mads
Yuya Nishihara - Oct. 17, 2014, 4:02 p.m.
On Fri, 17 Oct 2014 17:49:36 +0200, Mads Kiilerich wrote:
> On 10/17/2014 05:39 PM, Yuya Nishihara wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1413556690 -32400
> > #      Fri Oct 17 23:38:10 2014 +0900
> > # Node ID 1b432767e0829aba65a8f63bdcdb3bb9cabbce4f
> > # Parent  840be5ca03e1db16ba994e55597771c418166c97
> > cmdserver: add command to get pid of server handling current connection
> >
> > Since unix-mode server forks child process per request, client cannot know
> > the pid of the server, which is necessary to send SIGINT for example.
> >
> > Though Linux has getsockopt(SO_PEERCRED), it cannot be used because the server
> > fork()s after accept().  So commandserver should provide "getpid" command.
> 
> Wouldn't it be better to keep it more high level and just have a 'stop', 
> 'die' or 'kill' command?

Commandserver is blocking, it cannot receive new 'stop' command while handling
'runcommand' request.  So signal is the only way to interrupt stalled operation.

Regards,
Idan Kamara - Oct. 17, 2014, 5:37 p.m.
On Fri, Oct 17, 2014 at 8:39 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1413556690 -32400
> #      Fri Oct 17 23:38:10 2014 +0900
> # Node ID 1b432767e0829aba65a8f63bdcdb3bb9cabbce4f
> # Parent  840be5ca03e1db16ba994e55597771c418166c97
> cmdserver: add command to get pid of server handling current connection

Can't this go in the hello message?
Pierre-Yves David - Oct. 17, 2014, 9:29 p.m.
On 10/17/2014 08:39 AM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1413556690 -32400
> #      Fri Oct 17 23:38:10 2014 +0900
> # Node ID 1b432767e0829aba65a8f63bdcdb3bb9cabbce4f
> # Parent  840be5ca03e1db16ba994e55597771c418166c97
> cmdserver: add command to get pid of server handling current connection
>
> Since unix-mode server forks child process per request, client cannot know
> the pid of the server, which is necessary to send SIGINT for example.
>
> Though Linux has getsockopt(SO_PEERCRED), it cannot be used because the server
> fork()s after accept().  So commandserver should provide "getpid" command.

I'm not super excited about this, but I can see why this is necessary.

I've a bit confused about how you get:

(1) the child processing your request to reply to the same one
(2) the child processing your request to reply to this as it is blocked 
replying to the request.

I've technical feedback on the patch itself.

> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -212,6 +212,9 @@ class server(object):
>           """ writes the current encoding to the result channel """
>           self.cresult.write(encoding.encoding)
>
> +    def getpid(self):
> +        self.cresult.write(struct.pack('>i', os.getpid()))
> +

geeh, binary return? Seems not in line with the current command server 
architecture. Can we use ascii instead? (eg question: How is a bash 
script talking to command server expected to handle this binary encoded 
integer?)
Idan Kamara - Oct. 17, 2014, 10:12 p.m.
On Fri, Oct 17, 2014 at 2:29 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 10/17/2014 08:39 AM, Yuya Nishihara wrote:
>>
>> # HG changeset patch
>> # User Yuya Nishihara <yuya@tcha.org>
>> # Date 1413556690 -32400
>> #      Fri Oct 17 23:38:10 2014 +0900
>> # Node ID 1b432767e0829aba65a8f63bdcdb3bb9cabbce4f
>> # Parent  840be5ca03e1db16ba994e55597771c418166c97
>> cmdserver: add command to get pid of server handling current connection
>>
>> Since unix-mode server forks child process per request, client cannot know
>> the pid of the server, which is necessary to send SIGINT for example.
>>
>> Though Linux has getsockopt(SO_PEERCRED), it cannot be used because the
>> server
>> fork()s after accept().  So commandserver should provide "getpid" command.
>
>
> I'm not super excited about this, but I can see why this is necessary.
>
> I've a bit confused about how you get:
>
> (1) the child processing your request to reply to the same one
> (2) the child processing your request to reply to this as it is blocked
> replying to the request.

I believe the idea here is that a client connected to a socket mode server
wants a way to interrupt it by sending it a SIGINT, for which it needs its
pid.

So the flow is:

1) client connects to server
2) client runs command that hangs
3) user interrupts hung client
4) client sig handler can interrupt server since it now has its pid
Pierre-Yves David - Oct. 17, 2014, 10:21 p.m.
On 10/17/2014 03:12 PM, Idan Kamara wrote:
> On Fri, Oct 17, 2014 at 2:29 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 10/17/2014 08:39 AM, Yuya Nishihara wrote:
>>>
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1413556690 -32400
>>> #      Fri Oct 17 23:38:10 2014 +0900
>>> # Node ID 1b432767e0829aba65a8f63bdcdb3bb9cabbce4f
>>> # Parent  840be5ca03e1db16ba994e55597771c418166c97
>>> cmdserver: add command to get pid of server handling current connection
>>>
>>> Since unix-mode server forks child process per request, client cannot know
>>> the pid of the server, which is necessary to send SIGINT for example.
>>>
>>> Though Linux has getsockopt(SO_PEERCRED), it cannot be used because the
>>> server
>>> fork()s after accept().  So commandserver should provide "getpid" command.
>>
>>
>> I'm not super excited about this, but I can see why this is necessary.
>>
>> I've a bit confused about how you get:
>>
>> (1) the child processing your request to reply to the same one
>> (2) the child processing your request to reply to this as it is blocked
>> replying to the request.
>
> I believe the idea here is that a client connected to a socket mode server
> wants a way to interrupt it by sending it a SIGINT, for which it needs its
> pid.
>
> So the flow is:
>
> 1) client connects to server
> 2) client runs command that hangs
> 3) user interrupts hung client
> 4) client sig handler can interrupt server since it now has its pid

I got the main goal. But there is soem fork business going on. Is there 
a fork per connection of a fork per request ? if it is per connection I 
asssumes we have to get the pid at connection time for later use,
Matt Mackall - Oct. 17, 2014, 10:27 p.m.
On Fri, 2014-10-17 at 10:37 -0700, Idan Kamara wrote:
> On Fri, Oct 17, 2014 at 8:39 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1413556690 -32400
> > #      Fri Oct 17 23:38:10 2014 +0900
> > # Node ID 1b432767e0829aba65a8f63bdcdb3bb9cabbce4f
> > # Parent  840be5ca03e1db16ba994e55597771c418166c97
> > cmdserver: add command to get pid of server handling current connection
> 
> Can't this go in the hello message?

Seems like a better answer.
Yuya Nishihara - Oct. 18, 2014, 2:57 a.m.
On Fri, 17 Oct 2014 15:21:22 -0700, Pierre-Yves David wrote:
> On 10/17/2014 03:12 PM, Idan Kamara wrote:
> > On Fri, Oct 17, 2014 at 2:29 PM, Pierre-Yves David
> > <pierre-yves.david@ens-lyon.org> wrote:
> >> On 10/17/2014 08:39 AM, Yuya Nishihara wrote:
> >>>
> >>> # HG changeset patch
> >>> # User Yuya Nishihara <yuya@tcha.org>
> >>> # Date 1413556690 -32400
> >>> #      Fri Oct 17 23:38:10 2014 +0900
> >>> # Node ID 1b432767e0829aba65a8f63bdcdb3bb9cabbce4f
> >>> # Parent  840be5ca03e1db16ba994e55597771c418166c97
> >>> cmdserver: add command to get pid of server handling current connection
> >>>
> >>> Since unix-mode server forks child process per request, client cannot know
> >>> the pid of the server, which is necessary to send SIGINT for example.
> >>>
> >>> Though Linux has getsockopt(SO_PEERCRED), it cannot be used because the
> >>> server
> >>> fork()s after accept().  So commandserver should provide "getpid" command.
> >>
> >>
> >> I'm not super excited about this, but I can see why this is necessary.
> >>
> >> I've a bit confused about how you get:
> >>
> >> (1) the child processing your request to reply to the same one
> >> (2) the child processing your request to reply to this as it is blocked
> >> replying to the request.
> >
> > I believe the idea here is that a client connected to a socket mode server
> > wants a way to interrupt it by sending it a SIGINT, for which it needs its
> > pid.
> >
> > So the flow is:
> >
> > 1) client connects to server
> > 2) client runs command that hangs
> > 3) user interrupts hung client
> > 4) client sig handler can interrupt server since it now has its pid
> 
> I got the main goal. But there is soem fork business going on. Is there 
> a fork per connection of a fork per request ? if it is per connection I 
> asssumes we have to get the pid at connection time for later use,

A fork per connection.

So, the hello message will be better place.

On Fri, 17 Oct 2014 14:29:28 -0700, Pierre-Yves David wrote:
> > +    def getpid(self):
> > +        self.cresult.write(struct.pack('>i', os.getpid()))
> > +
> 
> geeh, binary return? Seems not in line with the current command server 
> architecture.

The return code of "runcommand" is int32_t.

Regards,

Patch

diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -212,6 +212,9 @@  class server(object):
         """ writes the current encoding to the result channel """
         self.cresult.write(encoding.encoding)
 
+    def getpid(self):
+        self.cresult.write(struct.pack('>i', os.getpid()))
+
     def serveone(self):
         cmd = self.client.readline()[:-1]
         if cmd:
@@ -226,7 +229,8 @@  class server(object):
         return cmd != ''
 
     capabilities = {'runcommand'  : runcommand,
-                    'getencoding' : getencoding}
+                    'getencoding' : getencoding,
+                    'getpid'      : getpid}
 
     def serve(self):
         hellomsg = 'capabilities: ' + ' '.join(sorted(self.capabilities))
diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t
--- a/tests/test-commandserver.t
+++ b/tests/test-commandserver.t
@@ -16,7 +16,7 @@ 
   ...     # run an arbitrary command to make sure the next thing the server
   ...     # sends isn't part of the hello message
   ...     runcommand(server, ['id'])
-  o, 'capabilities: getencoding runcommand\nencoding: *' (glob)
+  o, 'capabilities: getencoding getpid runcommand\nencoding: *' (glob)
   *** runcommand id
   000000000000 tip
 
@@ -519,6 +519,21 @@  check that local configs for the cached 
   prompt: 5678
 
 
+get pid of the server (same as subprocess' in pipe mode):
+
+  >>> import struct
+  >>> from hgclient import readchannel, check
+  >>> @check
+  ... def getpid(server):
+  ...     readchannel(server)
+  ...     server.stdin.write('getpid\n')
+  ...     ch, data = readchannel(server)
+  ...     print '%c, %r' % (ch, data)
+  ...     pid, = struct.unpack('>i', data)
+  ...     assert pid == server.pid
+  r, '*' (glob)
+
+
 start without repository:
 
   $ cd ..
@@ -531,7 +546,7 @@  start without repository:
   ...     # run an arbitrary command to make sure the next thing the server
   ...     # sends isn't part of the hello message
   ...     runcommand(server, ['id'])
-  o, 'capabilities: getencoding runcommand\nencoding: *' (glob)
+  o, 'capabilities: getencoding getpid runcommand\nencoding: *' (glob)
   *** runcommand id
   abort: there is no Mercurial repository here (.hg not found)
    [255]
@@ -562,7 +577,7 @@  unix domain socket:
   ...     print '%c, %r' % (ch, data)
   ...     runcommand(conn, ['id'])
   >>> check(hellomessage, server.connect)
-  o, 'capabilities: getencoding runcommand\nencoding: *' (glob)
+  o, 'capabilities: getencoding getpid runcommand\nencoding: *' (glob)
   *** runcommand id
   eff892de26ec tip bm1/bm2/bm3
   >>> def unknowncommand(conn):