Opened 4 years ago

Closed 4 years ago

#9376 closed enhancement (fixed)

subprocess management module for pyptlib

Reported by: infinity0 Owned by: asn
Priority: Medium Milestone:
Component: Obfuscation/Pluggable transport Version:
Severity: Keywords: pyptlib flashproxy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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

Child Tickets

Change History (14)

comment:1 Changed 4 years ago by infinity0

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

comment:2 Changed 4 years ago by asn

Status: newneeds_revision

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 a 

Seems 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_RUN is a global dict and its variable name is in capitals. Is there a special meaning to the capitals?
  • Wanna add a ChangeLog entry? :)

comment:3 Changed 4 years ago by infinity0

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.

comment:4 Changed 4 years ago by infinity0

edit fail; "it isn't user-visible" -> "the hack isn't user-visible"

comment:5 Changed 4 years ago by infinity0

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.

comment:6 Changed 4 years ago by infinity0

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); it should probably send either CTRL-BREAK or 2 CTRL-Cs to the PT instead.

comment:7 Changed 4 years ago by rransom

See #3049 for how controllers make Tor terminate when they exit (also note that #3049 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.

comment:8 Changed 4 years ago by infinity0

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

comment:9 Changed 4 years ago by asn

Looks good. Two things:

  • Nice tests! Any chance they could get some small documentation? For example, what is test_killal_kill supposed 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.py file to util/__init__.py. I don't like code in __init__.py except 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():
            YYY
    

I prefer the following:

def something_impl_windows():
       XXX

def something_impl():
       YYY

if mswindows:
     something = something_impl_windows
else:
     soemthing = something_impl

comment:10 in reply to:  9 Changed 4 years ago by infinity0

Replying to asn:

Looks good. Two things:

  • Nice tests! Any chance they could get some small documentation? For example, what is test_killal_kill supposed 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.py file to util/__init__.py. I don't like code in __init__.py except 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

or

if 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

comment:11 Changed 4 years ago by dcf

Keywords: flashproxy added

comment:12 Changed 4 years ago by infinity0

New compare link, includes the API move from pyptlib.util.checkClientMode to pyptlib.config.checkClientMode:

https://github.com/infinity0/pyptlib/compare/master...subproc

comment:13 Changed 4 years ago by asn

Hey, I see that by cd64f717c736a71b1128f5964e6adf5de8942561, you have killed the Makefile and updated the API.rst. Is there anything else I wanted you to do, or is this merge ready?

Thnaks!

comment:14 Changed 4 years ago by asn

Resolution: fixed
Status: needs_revisionclosed

Merged. Thanks!

Note: See TracTickets for help on using tickets.