add server.shutdown() method to SocketServer

[phr]

BPO	1193577
Nosy	@smontanaro, @akuchling
Superseder	bpo-2302: Uses of SocketServer.BaseServer.shutdown have a race
Files	issue1193577.patch: diff from current trunk, SocketServer and test_socketserver polling_shutdown.patch: werneck's patch, merged up to r61161 polling_shutdown.patch: timeouts reorganized; shutdown blocks polling_shutdown.patch: +documentation

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2008-03-16.17:20:30.103>
created_at = <Date 2005-05-02.03:04:36.000>
labels = ['type-feature', 'library']
title = 'add server.shutdown() method to SocketServer'
updated_at = <Date 2008-03-16.17:20:30.102>
user = 'https://bugs.python.org/phr'

bugs.python.org fields:

activity = <Date 2008-03-16.17:20:30.102>
actor = 'jyasskin'
assignee = 'jyasskin'
closed = True
closed_date = <Date 2008-03-16.17:20:30.103>
closer = 'jyasskin'
components = ['Library (Lib)']
creation = <Date 2005-05-02.03:04:36.000>
creator = 'phr'
dependencies = []
files = ['9553', '9579', '9581', '9586']
hgrepos = []
issue_num = 1193577
keywords = ['patch']
message_count = 9.0
messages = ['54518', '62804', '62994', '63170', '63172', '63177', '63185', '63345', '63587']
nosy_count = 7.0
nosy_names = ['skip.montanaro', 'akuchling', 'phr', 'pilcrow', 'jyasskin', 'zanella', 'werneck']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = '2302'
type = 'enhancement'
url = 'https://bugs.python.org/issue1193577'
versions = ['Python 2.6', 'Python 3.0']

[phr]

First of all the daemon_threads property on socket
servers should be added as an optional arg to the
server constructor, so you can say:
TCPServer(..., daemon_threads=True).serve_forever()

instead of

    temp = TCPServer(...)
    temp.daemon_threads=True
    temp.serve_forever

Secondly there should be a way to STOP the server, once
you've started serving forever! A request handler
should be able to say

   self.server.stop_serving()

This would work by setting a flag in the server object.
The serve_forever loop would use select with a timeout
(default 0.5 sec, also settable as an optional arg to
the server constructor) or alternatively set a timeout
on the socket object. So it would block listening for
new connections, but every 0.5 sec it would check the
stop_serving flag and break out of the loop if the flag
was set.

This method was suggested by Skip Montanaro in a clpy
thread about how to stop SocketServer and it's useful
enough that I think it should be part of the standard
impl. I can make a patch if it's not obvious how to do it.

[jyasskin]

This is getting in my way, so I'll take a look at it. I'm planning to
model the shutdown API after
http://java.sun.com/javase/6/docs/api/java/util/concurrent/ExecutorService.html.
The serve_forever->shutdown interval should probably also be available
through a context manager.

[werneck]

I had some code here to do the exact same thing with XML-RPC server. The
patch adds the feature to SocketServer, with a server.shutdown() method
to stop serve_forever(), and change the unittest to use and test the
feature.

I disagree on adding the daemon_threads arg as a keyword to TCPServer
since that's a feature of ThreadingMixIn

[jyasskin]

Polling isn't a great way to handle shutdown, since it wastes CPU time
and decreases responsiveness, but it's also easy and my attempt to avoid
it isn't getting anywhere, so I'm planning to fix up your patch a bit
and commit it. Thanks!

I've merged your patch with my conflicting change in the trunk and
re-attached it.

Two things:

1. This patch may interfere with the existing timeout in await_request.
   We may be able to re-use that instead of having two select statements.

2. I believe it's important to provide a way to block until the server
   isn't accepting any more requests and to block until all active requests
   are finished running. If the active requests depend on other bits of the
   system, blocking until they're done would help them terminate
   gracefully. It would also be useful to give users a more proactive way
   to kill active requests, perhaps by listing the handler objects
   associated with them (or their PIDs for forking servers). It's
   surprisingly complicated to avoid race conditions in these
   implementations, especially without support from the Server object.

[jyasskin]

Also, Pedro's argument against a daemon_threads argument to TCPServer
convinces me. I think we can add it in ThreadingMixIn.__init__ once this
whole hierarchy is switched over to new-style classes. That's already
done in 3.0, but I don't know what it would affect in 2.6. If anyone
wants to keep pushing it, would you open a new bug?

[jyasskin]

It seems that .await_request() was only added a month ago to fix issue
742598, so it's no great hardship to refactor it again now. Timeouts
never worked for .serve_forever() because the try/except in
.handle_request() turned their exception into a plain return, which
didn't stop .server_forever() from looping. Timeouts also seem to be
unnecessary in a situation in which shutdown works. Shutdown can only be
called from a separate thread, and when you have more threads you can
also do periodic tasks in them.

So I've made that explicit: the .serve_forever/shutdown combination
doesn't handle timeouts. [This has nothing to do with the fact that it
takes three times as much code to handle them. Don't look at the excuse
behind the curtain. ;)]

This patch needs some more documentation and a NEWS entry before it gets
submitted, but I want to check that everyone would be happy with the
concept.

[jyasskin]

I'll submit the attached patch in a couple days unless I get comments.

[jyasskin]

Hearing no objections, I've submitted this as r61289.

[jyasskin]

So this has a race. See bpo-2302 to discuss a fix.