Factor out thread setup/teardown, join threads by name, MacOS fixes

[ced, 2010-10-03 08:05:46]

http://codereview.appspot.com/2348042/diff/1/trytond/server.py
File trytond/server.py (right):

http://codereview.appspot.com/2348042/diff/1/trytond/server.py#newcode191
trytond/server.py:191: xmlrpcd.join()
Are you sure the main thread return only when all child thread are stopped?

[pheller, 2010-10-03 21:28:46]

On 2010/10/03 08:05:46, ced wrote:
> http://codereview.appspot.com/2348042/diff/1/trytond/server.py
> File trytond/server.py (right):
> 
> http://codereview.appspot.com/2348042/diff/1/trytond/server.py#newcode191
> trytond/server.py:191: xmlrpcd.join()
> Are you sure the main thread return only when all child thread are stopped?

Yes, the main thread will block on the join() calls until the child thread
finishes.

I tested by adding time.sleep(10) after the "while self.running" in netrpc, and
some print messages in server.py.  When I send SIGINT, stop() is called, join()
is called (~ 10 seconds pass), then join() returns.

--phil

[ced, 2010-10-03 21:35:35]

On 2010/10/03 21:28:46, pheller wrote:
> On 2010/10/03 08:05:46, ced wrote:
> > http://codereview.appspot.com/2348042/diff/1/trytond/server.py
> > File trytond/server.py (right):
> > 
> > http://codereview.appspot.com/2348042/diff/1/trytond/server.py#newcode191
> > trytond/server.py:191: xmlrpcd.join()
> > Are you sure the main thread return only when all child thread are stopped?
> 
> Yes, the main thread will block on the join() calls until the child thread
> finishes.
> 
> I tested by adding time.sleep(10) after the "while self.running" in netrpc,
and
> some print messages in server.py.  When I send SIGINT, stop() is called,
join()
> is called (~ 10 seconds pass), then join() returns.

I know it works for netrpc. But my concern is about others that use 
SocketServer.TCPServer (or derivate).

[pheller, 2010-10-04 18:22:26]

I've expanded the scope of this a little bit:

The service threads all have very similar __init__, stop, and run, so I have
factored those into a new trytond.protocols.common.daemon class

I've moved the socket shutdown/closure to the end of the run() method for these
classes.  This will prevent attempts to shutdown a socket that has already been
closed (and therefore file descriptor missing).  This happens on Mac OS.

trytond.protocols.common.endsocket() shutsdown and closes socket, ignoring
errno.ENOTCONN, which the Mac raises due to a python bug.

The threads now all have names, which makes things more obvious when debugging
with pdb (or eclipse or whatever).

I've tested this on MacOS, and it seems to work just fine.

As to the concern that sockets might not get shutdown if the threads aren't
joined via threading.enumerate, it is now easy to test: put a print after call
to "endsocket()" in trytond.protocols.common.daemon run() method before the
return.

Thoughts?

[ced, 2010-10-04 20:27:24]

http://codereview.appspot.com/2348042/diff/6001/trytond/protocols/netrpc.py
File trytond/protocols/netrpc.py (right):

http://codereview.appspot.com/2348042/diff/6001/trytond/protocols/netrpc.py#n...
trytond/protocols/netrpc.py:17: threading.Thread.__init__(self,
name='NetRPCClientThread')
Is there any issue about having same name?

http://codereview.appspot.com/2348042/diff/6001/trytond/protocols/webdav.py
File trytond/protocols/webdav.py (right):

http://codereview.appspot.com/2348042/diff/6001/trytond/protocols/webdav.py#n...
trytond/protocols/webdav.py:3: #import threading
Don't comment

http://codereview.appspot.com/2348042/diff/6001/trytond/protocols/webdav.py#n...
trytond/protocols/webdav.py:31: import trytond.protocols.common
unused

[pheller, 2010-10-04 21:17:41]

On 2010/10/04 20:27:24, ced wrote:
> http://codereview.appspot.com/2348042/diff/6001/trytond/protocols/netrpc.py
> File trytond/protocols/netrpc.py (right):
> 
>
http://codereview.appspot.com/2348042/diff/6001/trytond/protocols/netrpc.py#n...
> trytond/protocols/netrpc.py:17: threading.Thread.__init__(self,
> name='NetRPCClientThread')
> Is there any issue about having same name?

Tested with 3 simultaneous NetRPC Clients running, no problems.

Regarding the other issues, I will fix them in the next update on codereview.

Regarding the concern that client threads might terminate rather than block
join() on the server thread, research reveals that the SocketServer
ThreadingMixIn treats children as DaemonThreads=False by default, which means
they will block join() on the parent thread until they terminate.  So that is no
problem.

--phil

[ced, 2010-10-06 20:19:39]

Ok for me