Patchwork [2,of,2,V2] commandserver: handle backlog before exiting

login
register
mail settings
Submitter Jun Wu
Date Feb. 8, 2017, 10:52 p.m.
Message ID <cb56fce57eceef2cf4cd.1486594351@x1c>
Download mbox | patch
Permalink /patch/18356/
State Accepted
Headers show

Comments

Jun Wu - Feb. 8, 2017, 10:52 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1486593930 28800
#      Wed Feb 08 14:45:30 2017 -0800
# Node ID cb56fce57eceef2cf4cd9893d387b9fe2b3cecd6
# Parent  5fc577761fb78168fcbd7ec93d911a1b7b4989c9
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r cb56fce57ece
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 stop
queuing new connections and processing all pending connections before exit.
Augie Fackler - Feb. 10, 2017, 2:50 a.m.
On Wed, Feb 08, 2017 at 02:52:31PM -0800, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1486593930 28800
> #      Wed Feb 08 14:45:30 2017 -0800
> # Node ID cb56fce57eceef2cf4cd9893d387b9fe2b3cecd6
> # Parent  5fc577761fb78168fcbd7ec93d911a1b7b4989c9
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r cb56fce57ece
> commandserver: handle backlog before exiting

Queued these, thanks.

> 
> 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 stop
> queuing new connections and processing all pending connections before exit.
> 
> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -478,9 +478,21 @@ class unixforkingservice(object):
> 
>     def _mainloop(self):
> +        exiting = False
>         h = self._servicehandler
> -        while not h.shouldexit():
> +        while True:
> +            if not exiting and h.shouldexit():
> +                # clients can no longer connect() to the domain socket, so
> +                # we stop queuing new requests.
> +                # for requests that are queued (connect()-ed, but haven't been
> +                # accept()-ed), handle them before exit. otherwise, clients
> +                # waiting for recv() will receive ECONNRESET.
> +                self._servicehandler.unlinksocket(self.address)
> +                exiting = True
>             try:
>                 ready = select.select([self._sock], [], [], h.pollinterval)[0]
>                 if not ready:
> +                    # only exit if we completed all queued requests
> +                    if exiting:
> +                        break
>                     continue
>                 conn, _addr = self._sock.accept()
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Feb. 10, 2017, 1:11 p.m.
On Wed, 8 Feb 2017 14:52:31 -0800, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1486593930 28800
> #      Wed Feb 08 14:45:30 2017 -0800
> # Node ID cb56fce57eceef2cf4cd9893d387b9fe2b3cecd6
> # Parent  5fc577761fb78168fcbd7ec93d911a1b7b4989c9
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r cb56fce57ece
> 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 stop
> queuing new connections and processing all pending connections before exit.
> 
> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -478,9 +478,21 @@ class unixforkingservice(object):
>  
>      def _mainloop(self):
> +        exiting = False
>          h = self._servicehandler
> -        while not h.shouldexit():
> +        while True:
> +            if not exiting and h.shouldexit():
> +                # clients can no longer connect() to the domain socket, so
> +                # we stop queuing new requests.
> +                # for requests that are queued (connect()-ed, but haven't been
> +                # accept()-ed), handle them before exit. otherwise, clients
> +                # waiting for recv() will receive ECONNRESET.
> +                self._servicehandler.unlinksocket(self.address)

Missed setting self._socketunlinked. Perhaps you want self._unlinksocket().

Should I fix it in flight?
Jun Wu - Feb. 10, 2017, 1:14 p.m.
Excerpts from Yuya Nishihara's message of 2017-02-10 22:11:26 +0900:
> Missed setting self._socketunlinked. Perhaps you want self._unlinksocket().
> 
> Should I fix it in flight?

Sorry! It should be "self._unlinksocket()". Thanks for finding it out!
Yuya Nishihara - Feb. 10, 2017, 1:28 p.m.
On Fri, 10 Feb 2017 05:14:20 -0800, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-02-10 22:11:26 +0900:
> > Missed setting self._socketunlinked. Perhaps you want self._unlinksocket().
> > 
> > Should I fix it in flight?
> 
> Sorry! It should be "self._unlinksocket()". Thanks for finding it out!

Fixed and pushed.

Patch

diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -478,9 +478,21 @@  class unixforkingservice(object):
 
     def _mainloop(self):
+        exiting = False
         h = self._servicehandler
-        while not h.shouldexit():
+        while True:
+            if not exiting and h.shouldexit():
+                # clients can no longer connect() to the domain socket, so
+                # we stop queuing new requests.
+                # for requests that are queued (connect()-ed, but haven't been
+                # accept()-ed), handle them before exit. otherwise, clients
+                # waiting for recv() will receive ECONNRESET.
+                self._servicehandler.unlinksocket(self.address)
+                exiting = True
             try:
                 ready = select.select([self._sock], [], [], h.pollinterval)[0]
                 if not ready:
+                    # only exit if we completed all queued requests
+                    if exiting:
+                        break
                     continue
                 conn, _addr = self._sock.accept()