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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2011-07-25 19:38 |
Patch in attachment (tests still missing).
|
msg141126 - (view) |
Author: Giampaolo Rodola' (giampaolo.rodola) * |
Date: 2011-07-25 22:43 |
New patch including tests.
|
msg141127 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
msg394580 - (view) |
Author: Irit Katriel (iritkatriel) * |
Date: 2021-05-27 17:41 |
This was added to the documentation under issue39797.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:19 | admin | set | github: 56672 |
2021-05-27 17:41:24 | iritkatriel | set | status: open -> closed
nosy:
+ iritkatriel messages:
+ msg394580
resolution: out of date stage: patch review -> resolved |
2016-02-11 03:26:40 | martin.panter | set | nosy:
+ martin.panter messages:
+ msg260070
|
2013-04-07 20:40:55 | tshepang | set | nosy:
+ tshepang
|
2011-12-26 15:52:19 | Arcege | set | files:
+ simpletest.py
messages:
+ msg150264 |
2011-12-26 15:48:46 | Arcege | set | files:
+ socketserver.patch
messages:
+ msg150263 versions:
+ Python 3.2 |
2011-12-23 01:40:16 | Arcege | set | files:
+ simple.py versions:
+ Python 2.6, - Python 3.2, Python 3.3 nosy:
+ Arcege
messages:
+ msg150126
|
2011-08-09 03:08:43 | socketpair | set | messages:
+ msg141808 |
2011-07-26 21:34:27 | pitrou | set | messages:
+ msg141185 |
2011-07-26 21:28:41 | neologix | set | messages:
+ msg141184 |
2011-07-26 20:33:52 | pitrou | set | messages:
+ msg141178 |
2011-07-26 16:52:20 | socketpair | set | messages:
+ msg141170 |
2011-07-26 16:39:29 | socketpair | set | messages:
+ msg141169 |
2011-07-26 16:28:36 | pitrou | set | messages:
+ msg141168 |
2011-07-26 16:25:14 | socketpair | set | messages:
+ msg141167 |
2011-07-25 23:07:33 | pitrou | set | nosy:
+ pitrou messages:
+ msg141127
|
2011-07-25 22:43:10 | giampaolo.rodola | set | files:
+ socketserver.patch
messages:
+ msg141126 |
2011-07-25 19:38:16 | giampaolo.rodola | set | files:
+ socketserver.patch
messages:
+ msg141116 |
2011-07-25 18:37:01 | giampaolo.rodola | set | messages:
+ msg141110 |
2011-07-25 18:17:39 | petri.lehtinen | set | messages:
+ msg141109 |
2011-07-25 17:35:43 | neologix | set | nosy:
+ neologix, giampaolo.rodola messages:
+ msg141105
|
2011-07-25 17:14:51 | socketpair | set | messages:
+ msg141104 |
2011-07-25 11:16:58 | petri.lehtinen | set | files:
+ 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:52 | petri.lehtinen | set | nosy:
+ petri.lehtinen
|
2011-07-01 17:50:21 | santoso.wijaya | set | nosy:
+ santoso.wijaya
|
2011-07-01 10:46:37 | socketpair | create | |