[Python-Dev] SocketServer issues

[Guido van Rossum]

2012/3/13 Kristján Valur Jónsson <kristjan at ccpgames.com>:
> I want to mention some issues I‘ve had with the socketserver module, and
> discuss if there‘s a way to make it nicer.
>
> So, for a long time we were able to create magic stackless mixin classes for
> it, like ThreadingMixIn, and assuming we had the appropriate socket
> replacement library, be able to use it nicely using tasklets.
>
> Then, at some point, the run_forever loop was changed to support timeout
> through the use of select.select() before every socket.accept() call.  This
> was very awkward because the whole concept of select() really goes contrary
> to the approach of using microthreads, non-blocking IO and all that.

I'm surprised -- surely a non-blocking framework should have no
problem implementing select(), especially if it's for one file
descriptor?

> The only reason for this select call, was to support timeout for the
> accept.  And even for vanilla applications, it necessiates an extra kernel
> call for every accept, just for the timeout.

I think it's fine to change the code so that if poll_interval is
explicitly set to None, the select() call is skipped. I don't think
the default should change though. The select() call is normally needed
to support the shutdown() feature, which is very useful. And also the
overridable service_actions() method.

Oh, there's another select() call, in handle_request(), that should
also be skipped if timeout is None.

At least, I *think* a select() with a timeout of None blocks forever
or until the socket is ready or until it is interrupted; I think this
can always be skipped, since the immediately following I/O call will
block in exactly the same way. Unless the socket is set in
non-blocking mode; we may have to have provisions to avoid breaking
that situation too.

> The way around this for me has to do local modifications to the SocketServer
> and just get rid of the select.

I hope the above suggestion is sufficient? It's the best we can do
while maintaining backward compatibility. This class has a lot of
different features, and is designed to be subclassed, so it's hard to
make changes that don't break anything.

> So, my first question is:  Why not simply rely on the already built-in
> timeout support in the socket module?  Setting the correct timeout value on
> the accepting socket, will achieve the same thing.  Of course, one then has
> to reset the timeout value on the accepted socket, but this is minor.

I don't think it's the same thing at all. If you set a timeout on the
socket, the accept() or recvfrom() call in get_request() will raise an
exception if no request comes in within the timeout (default 0.5 sec);
with the timeout implemented in serve_forever(), get_request() and its
callers won't have to worry about the timeout exception.

> Second: Of late the SocketServer has grown additional features and
> attributes.  In particular, it now has two event objects, __shutdown_request
> and __is_shut_down.
>
> Notice the double underscores.
>
> They make it impossible to subclass the SocketServer class to provide a
> different implementation of run_forever().  Is there any good reason why
> these attributes have been made „private“ like this?  Having just seen
> Raymond‘s talk on how to subclass right, this looks like the wrong way to
> use the double leading underscores.

Assuming you meant serve_forever(), I see no problem at all. If you
override serve_forever() you also have to override shutdown(). That's
all. They are marked private because they are involved in subtle
invariants that are easily disturbed if users touch them.

I could live with making them single-underscore protected, only to be
used by knowledgeable subclasses. But not with making then public
attributes.

> So, two things really:
>
> The use of select.select in SocketServer makes it necessary to subclass it
> to write a new version of run_forever() for those that wish to use a
> non-blocking IO library instead of socket. And the presence of these private
> attributes make it (theoretically) impossible to specialize run_forever in a
> mix-in class.
>
>
>
> Any thoughs?  Is anyone interested in seeing how the timeouts can be done
> without using select.select()?  And what do you think about removing the
> double underscores from there and thus making serve_forever owerrideable?

Let's see a patch (based on my concerns above) and then we can talk again.

-- 
--Guido van Rossum (python.org/~guido)