Patchwork [2,of,2] commandserver: use selector2

login
register
mail settings
Submitter Jun Wu
Date July 15, 2017, 3:27 a.m.
Message ID <a6545c15171810059596.1500089247@x1c>
Download mbox | patch
Permalink /patch/22385/
State Accepted
Headers show

Comments

Jun Wu - July 15, 2017, 3:27 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1500089181 25200
#      Fri Jul 14 20:26:21 2017 -0700
# Node ID a6545c15171810059596259f60574011184c3412
# Parent  5664763de82b48ca6882bbb624d01d467b4920d0
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r a6545c151718
commandserver: use selector2

Previously, commandserver was using select.select. That could have issue if
_sock.fileno() >= FD_SETSIZE (usually 1024), which raises:

  ValueError: filedescriptor out of range in select()

We got that in production today, although it's the code opening that many
files to blame, it seems better for commandserver to work in this case.
There are multiple way to "solve" it, like preserving a fd with a small
number and swap it with sock using dup2(). But upgrading to a modern
selector supported by the system seems to be the most correct way.
Yuya Nishihara - July 15, 2017, 3:37 a.m.
On Fri, 14 Jul 2017 20:27:27 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1500089181 25200
> #      Fri Jul 14 20:26:21 2017 -0700
> # Node ID a6545c15171810059596259f60574011184c3412
> # Parent  5664763de82b48ca6882bbb624d01d467b4920d0
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r a6545c151718
> commandserver: use selector2
> 
> Previously, commandserver was using select.select. That could have issue if
> _sock.fileno() >= FD_SETSIZE (usually 1024), which raises:
> 
>   ValueError: filedescriptor out of range in select()

Can we get around it without vendoring selector2, by using poll() for example?
Jun Wu - July 15, 2017, 4:18 a.m.
Excerpts from Yuya Nishihara's message of 2017-07-15 12:37:39 +0900:
> On Fri, 14 Jul 2017 20:27:27 -0700, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1500089181 25200
> > #      Fri Jul 14 20:26:21 2017 -0700
> > # Node ID a6545c15171810059596259f60574011184c3412
> > # Parent  5664763de82b48ca6882bbb624d01d467b4920d0
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r a6545c151718
> > commandserver: use selector2
> > 
> > Previously, commandserver was using select.select. That could have issue if
> > _sock.fileno() >= FD_SETSIZE (usually 1024), which raises:
> > 
> >   ValueError: filedescriptor out of range in select()
> 
> Can we get around it without vendoring selector2, by using poll() for example?

poll is not available on OS X, which we need to support.

I tried to write some lightweight wrapper around them, then discovered I
was basically reinventing things. SocketServer has too many issues and is
over complicated for our usecase, but the Python 3 "selectors" interface
looks not that bad. So I think it's okay to reuse a mature library.

If it still looks too complicated, I think we can remove some selectors,
like removing JythonSelectSelector, epoll, devpoll.
Yuya Nishihara - July 15, 2017, 6:41 a.m.
On Fri, 14 Jul 2017 21:18:48 -0700, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-07-15 12:37:39 +0900:
> > On Fri, 14 Jul 2017 20:27:27 -0700, Jun Wu wrote:
> > > # HG changeset patch
> > > # User Jun Wu <quark@fb.com>
> > > # Date 1500089181 25200
> > > #      Fri Jul 14 20:26:21 2017 -0700
> > > # Node ID a6545c15171810059596259f60574011184c3412
> > > # Parent  5664763de82b48ca6882bbb624d01d467b4920d0
> > > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r a6545c151718
> > > commandserver: use selector2
> > > 
> > > Previously, commandserver was using select.select. That could have issue if
> > > _sock.fileno() >= FD_SETSIZE (usually 1024), which raises:
> > > 
> > >   ValueError: filedescriptor out of range in select()
> > 
> > Can we get around it without vendoring selector2, by using poll() for example?
> 
> poll is not available on OS X, which we need to support.

Oh, miserable fork of BSD.

> I tried to write some lightweight wrapper around them, then discovered I
> was basically reinventing things. SocketServer has too many issues and is
> over complicated for our usecase, but the Python 3 "selectors" interface
> looks not that bad. So I think it's okay to reuse a mature library.

Yeah, selectors API seems to do the right thing.

Queued, thanks.
Yuya Nishihara - July 15, 2017, 6:56 a.m.
On Fri, 14 Jul 2017 20:27:27 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1500089181 25200
> #      Fri Jul 14 20:26:21 2017 -0700
> # Node ID a6545c15171810059596259f60574011184c3412
> # Parent  5664763de82b48ca6882bbb624d01d467b4920d0
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r a6545c151718
> commandserver: use selector2

> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -23,4 +23,5 @@ from . import (
>      error,
>      pycompat,
> +    selectors2,
>      util,
>  )
> @@ -477,4 +478,6 @@ class unixforkingservice(object):
>          exiting = False
>          h = self._servicehandler
> +        selector = selectors2.DefaultSelector()
> +        selector.register(self._sock, selectors2.EVENT_READ)
>          while True:
>              if not exiting and h.shouldexit():
> @@ -487,5 +490,5 @@ class unixforkingservice(object):
>                  exiting = True
>              try:
> -                ready = select.select([self._sock], [], [], h.pollinterval)[0]
> +                ready = selector.select(timeout=h.pollinterval)
>                  if not ready:
>                      # only exit if we completed all queued requests

Perhaps we no longer need to catch select.error, but _syscall_wrapper() of
selectors2 seems to raise ETIMEDOUT depending on how timeout is detected.
Can you take a look?
Augie Fackler - July 15, 2017, 4:44 p.m.
> On Jul 15, 2017, at 2:41 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Fri, 14 Jul 2017 21:18:48 -0700, Jun Wu wrote:
>> Excerpts from Yuya Nishihara's message of 2017-07-15 12:37:39 +0900:
>>> On Fri, 14 Jul 2017 20:27:27 -0700, Jun Wu wrote:
>>>> # HG changeset patch
>>>> # User Jun Wu <quark@fb.com>
>>>> # Date 1500089181 25200
>>>> #      Fri Jul 14 20:26:21 2017 -0700
>>>> # Node ID a6545c15171810059596259f60574011184c3412
>>>> # Parent  5664763de82b48ca6882bbb624d01d467b4920d0
>>>> # Available At https://bitbucket.org/quark-zju/hg-draft 
>>>> #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r a6545c151718
>>>> commandserver: use selector2
>>>> 
>>>> Previously, commandserver was using select.select. That could have issue if
>>>> _sock.fileno() >= FD_SETSIZE (usually 1024), which raises:
>>>> 
>>>>  ValueError: filedescriptor out of range in select()
>>> 
>>> Can we get around it without vendoring selector2, by using poll() for example?
>> 
>> poll is not available on OS X, which we need to support.
> 
> Oh, miserable fork of BSD.
> 
>> I tried to write some lightweight wrapper around them, then discovered I
>> was basically reinventing things. SocketServer has too many issues and is
>> over complicated for our usecase, but the Python 3 "selectors" interface
>> looks not that bad. So I think it's okay to reuse a mature library.
> 
> Yeah, selectors API seems to do the right thing.

I agree - I had known about this, but forgot it existed. Kyle or Rodrigo hit the same select() issue recently, so I’m happy to see this fixed. Thanks!

> 
> Queued, thanks.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Sean Farley - July 15, 2017, 6:29 p.m.
Yuya Nishihara <yuya@tcha.org> writes:

> On Fri, 14 Jul 2017 21:18:48 -0700, Jun Wu wrote:
>> Excerpts from Yuya Nishihara's message of 2017-07-15 12:37:39 +0900:
>> > On Fri, 14 Jul 2017 20:27:27 -0700, Jun Wu wrote:
>> > > # HG changeset patch
>> > > # User Jun Wu <quark@fb.com>
>> > > # Date 1500089181 25200
>> > > #      Fri Jul 14 20:26:21 2017 -0700
>> > > # Node ID a6545c15171810059596259f60574011184c3412
>> > > # Parent  5664763de82b48ca6882bbb624d01d467b4920d0
>> > > # Available At https://bitbucket.org/quark-zju/hg-draft 
>> > > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r a6545c151718
>> > > commandserver: use selector2
>> > > 
>> > > Previously, commandserver was using select.select. That could have issue if
>> > > _sock.fileno() >= FD_SETSIZE (usually 1024), which raises:
>> > > 
>> > >   ValueError: filedescriptor out of range in select()
>> > 
>> > Can we get around it without vendoring selector2, by using poll() for example?
>> 
>> poll is not available on OS X, which we need to support.
>
> Oh, miserable fork of BSD.

For those curious, as I was, here is an article explaining which macOS
versions has a working version:

https://daniel.haxx.se/blog/2016/10/11/poll-on-mac-10-12-is-broken/

tl;dr Sierra was only fixed this last January.

Patch

diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -23,4 +23,5 @@  from . import (
     error,
     pycompat,
+    selectors2,
     util,
 )
@@ -477,4 +478,6 @@  class unixforkingservice(object):
         exiting = False
         h = self._servicehandler
+        selector = selectors2.DefaultSelector()
+        selector.register(self._sock, selectors2.EVENT_READ)
         while True:
             if not exiting and h.shouldexit():
@@ -487,5 +490,5 @@  class unixforkingservice(object):
                 exiting = True
             try:
-                ready = select.select([self._sock], [], [], h.pollinterval)[0]
+                ready = selector.select(timeout=h.pollinterval)
                 if not ready:
                     # only exit if we completed all queued requests