subprocess management module for pyptlib
Developed as part of work on the obfs/flashproxy combined plugin, but can be useful for other plugins. (The concept is general enough to go into python proper but that process will take much more time.)
https://github.com/infinity0/pyptlib/compare/upstream...master
Activity
ATOW the autoTerminateChildren() behaviour triggers on a single INT signal, which is actually against the pluggable transport spec; I will fix this.
https://gitweb.torproject.org/torspec.git/blob/HEAD:/proposals/180-pluggable-transport.txt#l434
Code looks good. Some comments:
-
The code logic looks reasonable, but some tests would give us much more confidence. Can you write integration tests that spin up a bunch of processes, killall them, SIGINT them, and check that the code works as intended?
+a = inspect.getargspec(subprocess.Popen.__init__) +_Popen_defaults = zip(a.args[-len(a.defaults):],a.defaults); del aSeems like the above code peaks into the original subprocess module and gets the default Popen options, which are later used in your mock Popen. Is there a cleaner/standard way of doing this?
-
_SIGINT_RUNis a global dict and its variable name is in capitals. Is there a special meaning to the capitals? -
Wanna add a ChangeLog entry? :)
Trac:
Status: new to needs_revision-
Wrote some tests, see the compare link again :)
Instead of inspect.getargsec, I could instead copy the method signature from subprocess.py but that might need updating with successive versions of python. It would be less of a hack, it isn't really user-visible or abusable so I'd rather go with it as it's more adaptable.
The _CAPITALS are meant to denote data structures that isn't supposed to change that much, but I can change it if you want.
Will add a Changelog.
Turns out you can't send SIGINT on windows (python throws OSError), and python transforms SIGTERM on windows into TerminateProcess (in subprocess.py, it defines kill = terminate for windows platforms) which behaves like a SIGKILL.
One way of filling in the gaps is to have subproc manually translate SIGINT into a CTRL_C and SIGTERM into a CTRL_BREAK, both of which are possible in Python. This is the most cross-platform solution even though it is non-standard (and no such standard exists). Let me know if you want me to proceed with this. The other, simpler option is just to ditch signal handling / graceful shutdown on windows, including the "2-INT shutdown" behaviour that is part of the PT spec.
I hate Windows.
I spoke too soon; there is no way to catch CTRL-C on windows in python 2.7: http://stackoverflow.com/questions/16686510/how-do-i-capture-sigint-in-python-on-windows
And the corresponding python bug is marked wontfix: http://bugs.python.org/issue18040 but perhaps we can persuade them to accept it...
I have not tried catching CTRL-BREAK yet; there is a signal.SIGBREAK on windows python but it's undocumented.
At the moment windows Tor kills PTs using TerminateProcess (see #9330 (moved)); it should probably send either CTRL-BREAK or 2 CTRL-Cs to the PT instead.
See #3049 (moved) for how controllers make Tor terminate when they exit (also note that #3049 (moved) is more reliable than having the controlling process try to send a signal to terminate its children). That still seems to be the least complex IPC mechanism that can provide this feature on Windows.
Just tested catching signal.CTRL_BREAK_EVENT on windows python and it works fine, as long as you set creationflags=CREATE_NEW_PROCESS_GROUP (see [1] and [2]). You send the signal with os.kill(pid, signal.CTRL_BREAK_EVENT) and handle the signal with signal.signal(signal.SIGBREAK, handler).
I'd suggest we amend the PT spec to explicitly say "Tor will try to shutdown a PT by sending it SIGTERM (UNIX) or CTRL_BREAK_EVENT (windows)" and add cross-platform method to this python module - e.g. pt_request_terminate() to send SIGTERM/SIGBREAK and pt_handle_terminate() to trap it.
If python fix that CTRL-C handling bug, we can extend this to the 2-INT (UNIX) / 2-CTRL-C (windows) shutdown.
@rransom: your option sounds like it would require the child to poll the parent and do cleanup itself, is that correct? I don't think that is a good design (despite the fact that Tor itself supports it) - it's not feasible for us to expect all child processes to implement this behaviour (you might want to Popen non-PT 3rd-party processes e.g. socat) and it involves unnecessarily ownership transferral (ownership of the child passes from the parent to the child itself) which adds complexity.
[1] http://docs.python.org/2/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP [2] http://bugs.python.org/issue9524
-
Looks good. Two things:
-
Nice tests! Any chance they could get some small documentation? For example, what is
test_killal_killsupposed to do, and how does it do it? It doesn't send any signals or anything. -
You didn't document the
_Popen_defaults = zip(a.args[-len(a.defaults):],a.defaults); del a. Can you document it?
Some further comments. If you don't have time to do these, I can fix them myself before merging:
-
Hm. It seems like you moved all the
util.pyfile toutil/__init__.py. I don't like code in__init__.pyexcept if there is a reason. Is there a reason for it? -
I'm not really used to the following idiom (wrt proc_is_alive()):
if mswindows: def something(): XXX else: def something(): YYYI prefer the following:
def something_impl_windows(): XXX def something_impl(): YYY if mswindows: something = something_impl_windows else: soemthing = something_impl -
Replying to asn:
Looks good. Two things:
-
Nice tests! Any chance they could get some small documentation? For example, what is
test_killal_killsupposed to do, and how does it do it? It doesn't send any signals or anything. -
You didn't document the
_Popen_defaults = zip(a.args[-len(a.defaults):],a.defaults); del a. Can you document it?
Fixed.
Some further comments. If you don't have time to do these, I can fix them myself before merging:
- Hm. It seems like you moved all the
util.pyfile toutil/__init__.py. I don't like code in__init__.pyexcept if there is a reason. Is there a reason for it?
Python needs init.py to find the submodules (here, subproc.py). If you don't want code in init.py, you can move everything to say, util/util.py then add aliases in util/init.py (to avoid breaking previous clients), but I don't think it's worth the effort here. I can add a note for future developers though. Actually util.checkClientMode() could be moved into config.py already, but perhaps it would be cleaner to get rid of it altogether. I don't know how many others use it.
- I'm not really used to the following idiom (wrt proc_is_alive()):
if mswindows: def something(): XXX else: def something(): YYY }}} I prefer the following: {{{ def something_impl_windows(): XXX def something_impl(): YYY if mswindows: something = something_impl_windows else: soemthing = something_impl }}}Unfortunately we do some platform-specific imports as well so it's not as simple as that. Either I would have to do
{{{ try: import windows-specific except ImportError pass
try: import others except ImportError pass
then what you said
orif mswindows: import windows-specific else: import others
then what you said
neither of which seem that much cleaner. If it helps, python's subprocess.py do a similar thing to me and even do a conditional "def" in the middle of the Popen class itself. :p-
New compare link, includes the API move from pyptlib.util.checkClientMode to pyptlib.config.checkClientMode:
https://github.com/infinity0/pyptlib/compare/master...subproc