Patchwork [stable] commandserver: handle backlog before exiting

login
register
mail settings
Submitter Jun Wu
Date Jan. 30, 2017, 4:52 p.m.
Message ID <0f9c5c49ad7ac321f6fe.1485795166@x1c>
Download mbox | patch
Permalink /patch/18283/
State Deferred
Headers show

Comments

Jun Wu - Jan. 30, 2017, 4:52 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1485794838 0
#      Mon Jan 30 16:47:18 2017 +0000
# Node ID 0f9c5c49ad7ac321f6fecb4ca90121969bb038d4
# Parent  ea5353feeec3c2eddd7e4acfd398baddaf37b3e4
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 0f9c5c49ad7a
commandserver: handle backlog before exiting

Previously, when a chg server is exiting, it does not handle connected
clients so clients may get ECONNRESET and crash:

  1. client connect() # success
  2. server shouldexit = True and exit
  3. client recv() # ECONNRESET

d7875bfbfccb makes this race condition easier to reproduce if a lot of short
chg commands are started in parallel.

This patch fixes the above issue by unlinking the socket path to prevent new
connections and handling all pending connections before exiting.
Jun Wu - Jan. 31, 2017, 9:52 a.m.
Excerpts from Jun Wu's message of 2017-01-30 16:52:46 +0000:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1485794838 0
> #      Mon Jan 30 16:47:18 2017 +0000
> # Node ID 0f9c5c49ad7ac321f6fecb4ca90121969bb038d4
> # Parent  ea5353feeec3c2eddd7e4acfd398baddaf37b3e4
> # Available At https://bitbucket.org/quark-zju/hg-draft 
> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 0f9c5c49ad7a
> commandserver: handle backlog before exiting
> 
> Previously, when a chg server is exiting, it does not handle connected
> clients so clients may get ECONNRESET and crash:
> 
>   1. client connect() # success
>   2. server shouldexit = True and exit
>   3. client recv() # ECONNRESET
> 
> d7875bfbfccb makes this race condition easier to reproduce if a lot of short
> chg commands are started in parallel.
> 
> This patch fixes the above issue by unlinking the socket path to prevent new
> connections and handling all pending connections before exiting.
> 
> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -412,5 +412,9 @@ class unixservicehandler(object):
>  
>      def unlinksocket(self, address):
> -        os.unlink(address)
> +        try:
> +            os.unlink(address)
> +        except OSError as ex:
> +            if ex.errno != errno.ENOENT:
> +                raise

This introduces another race condition that unlinks an innocent socket file.
I will send a V2 later.

>  
>      def printbanner(self, address):
> @@ -471,9 +475,17 @@ class unixforkingservice(object):
>  
>      def _mainloop(self):
> +        exiting = False
>          h = self._servicehandler
> -        while not h.shouldexit():
> +        while True:
> +            if not exiting and h.shouldexit():
> +                # prevent accepting new connections
> +                self._servicehandler.unlinksocket(self.address)
> +                exiting = True
>              try:
>                  ready = select.select([self._sock], [], [], h.pollinterval)[0]
>                  if not ready:
> +                    # only exit if we have no more connections to handle
> +                    if exiting:
> +                        break
>                      continue
>                  conn, _addr = self._sock.accept()
Yuya Nishihara - Jan. 31, 2017, 2:36 p.m.
On Tue, 31 Jan 2017 09:52:51 +0000, Jun Wu wrote:
> Excerpts from Jun Wu's message of 2017-01-30 16:52:46 +0000:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1485794838 0
> > #      Mon Jan 30 16:47:18 2017 +0000
> > # Node ID 0f9c5c49ad7ac321f6fecb4ca90121969bb038d4
> > # Parent  ea5353feeec3c2eddd7e4acfd398baddaf37b3e4
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 0f9c5c49ad7a
> > commandserver: handle backlog before exiting
> > 
> > Previously, when a chg server is exiting, it does not handle connected
> > clients so clients may get ECONNRESET and crash:
> > 
> >   1. client connect() # success
> >   2. server shouldexit = True and exit
> >   3. client recv() # ECONNRESET
> > 
> > d7875bfbfccb makes this race condition easier to reproduce if a lot of short
> > chg commands are started in parallel.
> > 
> > This patch fixes the above issue by unlinking the socket path to prevent new
> > connections and handling all pending connections before exiting.
> > 
> > diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> > --- a/mercurial/commandserver.py
> > +++ b/mercurial/commandserver.py
> > @@ -412,5 +412,9 @@ class unixservicehandler(object):
> >  
> >      def unlinksocket(self, address):
> > -        os.unlink(address)
> > +        try:
> > +            os.unlink(address)
> > +        except OSError as ex:
> > +            if ex.errno != errno.ENOENT:
> > +                raise
> 
> This introduces another race condition that unlinks an innocent socket file.
> I will send a V2 later.

I have no idea if we can reliably make the socket stop queuing new connections
without destroying the backlog.

Instead, maybe the client can handle very first ECONNRESET gracefully?
Jun Wu - Jan. 31, 2017, 3:24 p.m.
Excerpts from Yuya Nishihara's message of 2017-01-31 23:36:46 +0900:
> > This introduces another race condition that unlinks an innocent socket file.
> > I will send a V2 later.

(This actually seems to be a regression after the socketserver refactoring.
It probably requires sub-classing "unixforkingservice" in chg to solve
properly. It's not too trivial and I'll send fixes after the freeze.)


> I have no idea if we can reliably make the socket stop queuing new connections
> without destroying the backlog.
> 
> Instead, maybe the client can handle very first ECONNRESET gracefully?

After "unlink", the client cannot "connect" by a filesystem address. And chg
client does connect by a filesystem address. So it should be reliable (not
considering Linux-specific black magic /proc/random-pid/fd/n).

I think the "industry standard" about shutting down is to handle all
remaining requests while rejecting new ones. The pattern is seen in nginx,
squid, thin (a Ruby on Rails server), OpenStack APIs etc. It looks better
than retrying in the client.
Yuya Nishihara - Jan. 31, 2017, 3:48 p.m.
On Tue, 31 Jan 2017 15:24:42 +0000, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-01-31 23:36:46 +0900:
> > > This introduces another race condition that unlinks an innocent socket file.
> > > I will send a V2 later.
> 
> (This actually seems to be a regression after the socketserver refactoring.
> It probably requires sub-classing "unixforkingservice" in chg to solve
> properly. It's not too trivial and I'll send fixes after the freeze.)

I don't think SocketServer does anything special for flushing backlog.

> > I have no idea if we can reliably make the socket stop queuing new connections
> > without destroying the backlog.
> > 
> > Instead, maybe the client can handle very first ECONNRESET gracefully?
> 
> After "unlink", the client cannot "connect" by a filesystem address. And chg
> client does connect by a filesystem address. So it should be reliable (not
> considering Linux-specific black magic /proc/random-pid/fd/n).

Yeah, but you found there's another race.

> I think the "industry standard" about shutting down is to handle all
> remaining requests while rejecting new ones. The pattern is seen in nginx,
> squid, thin (a Ruby on Rails server), OpenStack APIs etc.

If they do, we can copy. Can you investigate how they solve the problem?
I tried if shutdown(SHUT_RD) could be used, but it didn't help as expected.

> It looks better
> than retrying in the client.

Yes, but we have retry for connect(), so adding retry for the first recv()
wouldn't make things much worse.
Jun Wu - Jan. 31, 2017, 8:11 p.m.
Excerpts from Yuya Nishihara's message of 2017-02-01 00:48:26 +0900:
> On Tue, 31 Jan 2017 15:24:42 +0000, Jun Wu wrote:
> > Excerpts from Yuya Nishihara's message of 2017-01-31 23:36:46 +0900:
> > > > This introduces another race condition that unlinks an innocent socket file.
> > > > I will send a V2 later.
> > 
> > (This actually seems to be a regression after the socketserver refactoring.
> > It probably requires sub-classing "unixforkingservice" in chg to solve
> > properly. It's not too trivial and I'll send fixes after the freeze.)
> 
> I don't think SocketServer does anything special for flushing backlog.

Sorry, I missed chgunixservicehandler's customized "unlinksocket"
implementation. So chg should actually work just fine, while commandserver
needs some care to avoid double unlink (which is probably just adding an
"if" to prevent the second unlink).

> > > I have no idea if we can reliably make the socket stop queuing new connections
> > > without destroying the backlog.
> > > 
> > > Instead, maybe the client can handle very first ECONNRESET gracefully?
> > 
> > After "unlink", the client cannot "connect" by a filesystem address. And chg
> > client does connect by a filesystem address. So it should be reliable (not
> > considering Linux-specific black magic /proc/random-pid/fd/n).
> 
> Yeah, but you found there's another race.

That's a separated one (double unlink), not hard to solve.

> > I think the "industry standard" about shutting down is to handle all
> > remaining requests while rejecting new ones. The pattern is seen in nginx,
> > squid, thin (a Ruby on Rails server), OpenStack APIs etc.
> 
> If they do, we can copy. Can you investigate how they solve the problem?
> I tried if shutdown(SHUT_RD) could be used, but it didn't help as expected.

The epoll server (thin / eventmachine) uses "epoll_ctl (epfd, EPOLL_CTL_DEL,
...)". Squid code seems too complex. I guess "shutdown" is for connections
that are the return value of accept (3), while we want to deal with the
"socket" argument of accept (3), so shutdown does not help.

> > It looks better
> > than retrying in the client.
> 
> Yes, but we have retry for connect(), so adding retry for the first recv()
> wouldn't make things much worse.

That's actually interesting as it can probably cover ECONNREFUSED too. I'll
try if it could be implemented cleanly. That said, I would still like the
server-side fix which is simply effective.
Yuya Nishihara - Feb. 1, 2017, 1:45 p.m.
On Tue, 31 Jan 2017 20:11:09 +0000, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-02-01 00:48:26 +0900:
> > On Tue, 31 Jan 2017 15:24:42 +0000, Jun Wu wrote:
> > > Excerpts from Yuya Nishihara's message of 2017-01-31 23:36:46 +0900:
> > > > > This introduces another race condition that unlinks an innocent socket file.
> > > > > I will send a V2 later.
> > > 
> > > (This actually seems to be a regression after the socketserver refactoring.
> > > It probably requires sub-classing "unixforkingservice" in chg to solve
> > > properly. It's not too trivial and I'll send fixes after the freeze.)
> > 
> > I don't think SocketServer does anything special for flushing backlog.
> 
> Sorry, I missed chgunixservicehandler's customized "unlinksocket"
> implementation. So chg should actually work just fine, while commandserver
> needs some care to avoid double unlink (which is probably just adding an
> "if" to prevent the second unlink).

Ok, I think I got your idea, which is:

 on successful exit:
 1. unlink(sockpath) for pseudo closing
 2. accept() and fork() for each queued connection
 3. real close() at end
 # we have to avoid double unlink()s here

 on failure:
 0. someone raises exception
 1or2. unlink(sockpath)
 2or1. close()

That sounds good.

> > > I think the "industry standard" about shutting down is to handle all
> > > remaining requests while rejecting new ones. The pattern is seen in nginx,
> > > squid, thin (a Ruby on Rails server), OpenStack APIs etc.
> > 
> > If they do, we can copy. Can you investigate how they solve the problem?
> > I tried if shutdown(SHUT_RD) could be used, but it didn't help as expected.
> 
> The epoll server (thin / eventmachine) uses "epoll_ctl (epfd, EPOLL_CTL_DEL,
> ...)". Squid code seems too complex. I guess "shutdown" is for connections
> that are the return value of accept (3), while we want to deal with the
> "socket" argument of accept (3), so shutdown does not help.

I guess epoll_ctl() would just stop event emission and the kernel would
continue queuing new connection. Anyway, we can do unlink(sockpath) as you
said, so complex hack wouldn't be needed. Thanks for checking the real-world
implementation.

OT: typical use of shutdown() I know is to send FIN to close the read end
at the server side.

 client: shutdown(SHUT_WR)
 server: receives FIN and recv() ends with EOF
 server: sends response to client
 client: receives response from server
 client: close() the session

We could do shutdown(SHUT_RD) for listening socket, but which just prevented
us from calling accept()s. I just tried that in case there's some trick I
didn't know.

> > > It looks better
> > > than retrying in the client.
> > 
> > Yes, but we have retry for connect(), so adding retry for the first recv()
> > wouldn't make things much worse.
> 
> That's actually interesting as it can probably cover ECONNREFUSED too. I'll
> try if it could be implemented cleanly. That said, I would still like the
> server-side fix which is simply effective.

Agreed. Server-side fix would be cleaner.

Patch

diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -412,5 +412,9 @@  class unixservicehandler(object):
 
     def unlinksocket(self, address):
-        os.unlink(address)
+        try:
+            os.unlink(address)
+        except OSError as ex:
+            if ex.errno != errno.ENOENT:
+                raise
 
     def printbanner(self, address):
@@ -471,9 +475,17 @@  class unixforkingservice(object):
 
     def _mainloop(self):
+        exiting = False
         h = self._servicehandler
-        while not h.shouldexit():
+        while True:
+            if not exiting and h.shouldexit():
+                # prevent accepting new connections
+                self._servicehandler.unlinksocket(self.address)
+                exiting = True
             try:
                 ready = select.select([self._sock], [], [], h.pollinterval)[0]
                 if not ready:
+                    # only exit if we have no more connections to handle
+                    if exiting:
+                        break
                     continue
                 conn, _addr = self._sock.accept()