classification
Title: Calling SocketServer.shutdown() when server_forever() was not called will hang
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.2, Python 2.7, Python 2.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Arcege, giampaolo.rodola, martin.panter, neologix, petri.lehtinen, pitrou, santoso.wijaya, socketpair, tshepang
Priority: normal Keywords: needs review, patch

Created on 2011-07-01 10:46 by socketpair, last changed 2016-02-11 03:26 by martin.panter.

Files
File name Uploaded Description Edit
issue12463.patch petri.lehtinen, 2011-07-25 11:16 review
socketserver.patch giampaolo.rodola, 2011-07-25 19:38 review
socketserver.patch giampaolo.rodola, 2011-07-25 22:43 adds test review
simple.py Arcege, 2011-12-23 01:40 Start a http server that immediately calls server.shutdown()
socketserver.patch Arcege, 2011-12-26 15:48 Patch to fix shutdown() method
simpletest.py Arcege, 2011-12-26 15:52
Messages (21)
msg139565 - (view) Author: Марк Коренберг (socketpair) * Date: 2011-07-01 10:46
Why not to call self.__is_shut_down.set() in constructor ?

In reality, if event loop was not started, it mean that server is shut down.
msg141086 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-07-25 11:16
Attached a patch with a test and the proposed fix.
msg141104 - (view) Author: Марк Коренберг (socketpair) * Date: 2011-07-25 17:14
Yes, this is what I want. Thanks.
msg141105 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-25 17:35
Seems reasonable to me.
What bothers me is this comment in shutdown's docstring:

        """Stops the serve_forever loop.

        Blocks until the loop has finished. This must be called while
        serve_forever() is running in another thread, or it will
        deadlock.
        """

I'm adding Giampaolo to the nosy list, to get his input on this.
msg141109 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2011-07-25 18:17
OK, so this is intentional. But why? Why does it *have to* block on every call? It would be more reasonable to make in not block if serve_forever() is not running and for example add a function to check whether serve_forever() is running or not.

If for some reason it has to fail when serve_forever() is not running, a deadlock is still not good. An exception would be better.
msg141110 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-07-25 18:37
IMHO, both methods should raise an exception (RuntimeError maybe) in case the server is already started or stopped, at least in Python 3.3, while previous python versions should silently "pass".

It also makes sense to add a "running" property returning a bool, as suggested by Petri.
msg141116 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-07-25 19:38
Patch in attachment (tests still missing).
msg141126 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2011-07-25 22:43
New patch including tests.
msg141127 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-07-25 23:07
Can you explain the issue a bit more?
If thread A calls shutdown(), and thread B calls serve_forever() a bit later, thread A should IMO wait for serve_forever() to finish.
msg141167 - (view) Author: Марк Коренберг (socketpair) * Date: 2011-07-26 16:25
In my case, exception occured in separate thread between the begginig of the thread and starting loop.

Main thread has a code that correctly stops that separate thread. It just calls loop_thread.server.server_close() and after that, calls loop_thread.join()

So, if program termination required, but loop was not started, deadlock occur. Moreover, I can not detect if loop was started. And more more over, if I detect that loop was not started, there is chance, that it may be started a bit later, next after my checking. In that case, either loop will not be terminated correctly (if process exits) or join() may block.

So, please DO NOT ADD detection if loop was started. That will generate race-conditions in user code. I think it's never needed. If one wants to detect that, one may use separate variable:
{code}
started = True
xxx.serve_forever() 
{code}
msg141168 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-07-26 16:28
> In my case, exception occured in separate thread between the begginig
> of the thread and starting loop.
> 
> Main thread has a code that correctly stops that separate thread. It
> just calls loop_thread.server.server_close() and after that, calls
> loop_thread.join()
> 
> So, if program termination required, but loop was not started,
> deadlock occur.

Sorry, can you post a snippet reproducing the issue? It will make it
clearer for me to understand it.
msg141169 - (view) Author: Марк Коренберг (socketpair) * Date: 2011-07-26 16:39
Why you create variable mirror?
--------------------------
self.__running = False
self.__is_shut_down.set()
--------------------------

You other code is racy. exception may occur at any time.

Why not to test state of __is_shut_down directly ? In any case, server either running either not (i.e. is_shitdown or not is_shutdown). Server should not be tri-state in any case, as I think.

As the latest thing, that serve_forever does - is the calling  self.__is_shut_down.set(). So it is reliable to detect state of the server via state of this lock.
msg141170 - (view) Author: Марк Коренберг (socketpair) * Date: 2011-07-26 16:52
class ThreadedRPC(Thread, SimpleXMLRPCServer):
    def __init__(self):
        Thread.__init__(self)
        SimpleXMLRPCServer.__init__(self)
        # python bug workaround, which eliminate hang on test_exception
        self._BaseServer__is_shut_down.set()
        self.daemon = True

    def run(self):
        raise Exception('race-condition!')
        self.serve_forever()


def main():
    rpc = ThreadedRPC(RPC_LISTEN_ADDR, allow_none=True)
    rpc.start() # from Thread

    # term set by signal handler
    while rpc.isAlive() and not term:
        time.sleep(0.5)

    # note, thread may die in any time! regardles of isAlive()
    # if thread dead before calling serve_forever(), will hang here without a workaround
    rpc.shutdown() # from xmlrpcservr. 

    rpc.join() # from thread (no problem, as thread is not Alive)
msg141178 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-07-26 20:33
[...]
>     # note, thread may die in any time! regardles of isAlive()
>     # if thread dead before calling serve_forever(), will hang here without a workaround
>     rpc.shutdown() # from xmlrpcservr. 

Why do you say it "hangs"? It doesn't hang, it just waits for you to
call serve_forever(): it's not a bug, it's actually a feature.
msg141184 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-26 21:28
When you join a thread which hasn't been started, you get an
exception, not a deadlock.
When you join a newly-created queue, it doesn't wait until an item is
submitted to the queue.
But honestly I don't really care, since it's such a minor nuisance and
open to interpretation.
msg141185 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-07-26 21:34
> When you join a thread which hasn't been started, you get an
> exception, not a deadlock.

But we don't have a deadlock here: another thread could perfectly well
call serve_forever() in the meantime and shutdown() should then return
after having stopped the event loop.

My point is that changing semantics may break existing software, without
there being a clear benefit in doing so.
msg141808 - (view) Author: Марк Коренберг (socketpair) * Date: 2011-08-09 03:08
> Why do you say it "hangs"? It doesn't hang, it just waits for you to
> call serve_forever(): it's not a bug, it's actually a feature.

Okay, for my case, How I should correctly terminate thread?

Conditions:
1. signal may appear at any time
2. thread may become non-alive in any time - either before server_forever (due to exception) or after.

So, after breaking main loop, I should correctly "terminate" thread. I can not say if thread is running. checking isAlive is racy. Even when thread is alive, this is unknown if server_forever was called. Even if serve_forever was not called, we should not rely on calling this ones, because thread may be interrupted by exception before calling serve_forever.

Once again: why not to add patch suggested by Petri Lehtinen ?
msg150126 - (view) Author: Michael P. Reilly (Arcege) Date: 2011-12-23 01:40
I'm seeing that shutdown does have a race condition just using BaseHTTPServer.HTTPServer.  See the attached simple script.  Then access http://localhost:8081.  This is using both Python 2.6.6 (r266:84292, May 22 2011, 16:47:42) on Oracle Linux Server 6.1 and Python 2.7.2+ (default, Oct 4, 2011, 20:03:08) on Ubuntu 11.10.  I have applied the socketserver.patch dated 2011-07-25 18:43, with the same result.

The problem is that shutdown() waits for the event, which is never triggered since there is no thread to set the event.  I'll see if I can come up with a patch myself later this weekend.  I suspect the same might happen with ForkingMixIn.
msg150263 - (view) Author: Michael P. Reilly (Arcege) Date: 2011-12-26 15:48
Here is a patch to socketserver.py which can be applied to 2.6, 2.7 and 3.2.  The fix is for BaseServer, ForkingMixIn and ThreadingMixIn.  All three now correctly respond to the shutdown method.  I have no way of testing Windows or MacOSX (based on docs, MacOSX should work without changes); the ForkingMixIn will raise an AssertionError on non-POSIX systems.  There may be a better way of handling non-POSIX systems, but again, I'm not able to test or develop at this time.  I'll also update the simpletest.py script.
msg150264 - (view) Author: Michael P. Reilly (Arcege) Date: 2011-12-26 15:52
An update test program.  Execute with appropriate PYTHONPATH (to dir to patched module and explicit interpreter executable: PYTHONPATH=$PWD/2.7/b/Lib python2.7 $PWD/simpletest.py
msg260070 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-11 03:26
Michael’s patch looks like it adds some backwards compatibility problems. The forking server overwrites a signal handler, and shutdown() no longer seems to wait at all.

It seems that people want shutdown() to do different things. Mark doesn’t want it to wait for the server in case an exception stops serve_forever() from starting its loop. So he has to rely on join() rather than just shutdown() to ensure that serve_forever() will no longer run.

Antoine wants shutdown() to guarantee that serve_forever() has stopped. So he has to hope that no exception (e.g. KeyboardInterrupt) prevents serve_forever() from starting. I think I agree with this. If you want to do extra processing before calling serve_forever(), maybe use an exception handler, different thread, or synchronization mechanism to avoid the lockout problem instead. I suggest just documenting the way things work and leaving it at that.
History
Date User Action Args
2016-02-11 03:26:40martin.pantersetnosy: + martin.panter
messages: + msg260070
2013-04-07 20:40:55tshepangsetnosy: + tshepang
2011-12-26 15:52:19Arcegesetfiles: + simpletest.py

messages: + msg150264
2011-12-26 15:48:46Arcegesetfiles: + socketserver.patch

messages: + msg150263
versions: + Python 3.2
2011-12-23 01:40:16Arcegesetfiles: + simple.py
versions: + Python 2.6, - Python 3.2, Python 3.3
nosy: + Arcege

messages: + msg150126
2011-08-09 03:08:43socketpairsetmessages: + msg141808
2011-07-26 21:34:27pitrousetmessages: + msg141185
2011-07-26 21:28:41neologixsetmessages: + msg141184
2011-07-26 20:33:52pitrousetmessages: + msg141178
2011-07-26 16:52:20socketpairsetmessages: + msg141170
2011-07-26 16:39:29socketpairsetmessages: + msg141169
2011-07-26 16:28:36pitrousetmessages: + msg141168
2011-07-26 16:25:14socketpairsetmessages: + msg141167
2011-07-25 23:07:33pitrousetnosy: + pitrou
messages: + msg141127
2011-07-25 22:43:10giampaolo.rodolasetfiles: + socketserver.patch

messages: + msg141126
2011-07-25 19:38:16giampaolo.rodolasetfiles: + socketserver.patch

messages: + msg141116
2011-07-25 18:37:01giampaolo.rodolasetmessages: + msg141110
2011-07-25 18:17:39petri.lehtinensetmessages: + msg141109
2011-07-25 17:35:43neologixsetnosy: + neologix, giampaolo.rodola
messages: + msg141105
2011-07-25 17:14:51socketpairsetmessages: + msg141104
2011-07-25 11:16:58petri.lehtinensetfiles: + issue12463.patch
versions: - Python 2.6, Python 3.1, Python 3.4
messages: + msg141086

keywords: + patch, needs review
stage: patch review
2011-07-03 20:08:52petri.lehtinensetnosy: + petri.lehtinen
2011-07-01 17:50:21santoso.wijayasetnosy: + santoso.wijaya
2011-07-01 10:46:37socketpaircreate