Popen lacking context manager support

[arcivanov]

Per Python Docs Popen can be used as a context manager.

Popen objects are supported as context managers via the with statement: on exit, standard file descriptors are closed, and the process is waited for.

with Popen(["ifconfig"], stdout=PIPE) as proc:
    log.write(proc.stdout.read())

Changed in version 3.2: Added context manager support.

psutil Popen doesn't replicate this behavior:

Traceback (most recent call last):
  File "process_tests.py", line 244, in test_process_log_stream_handler
    with Popen(sys.executable, stdout=PIPE, stderr=PIPE) as proc:
AttributeError: __exit__

[arcivanov]

Also, @giampaolo, FYI, this happens because stdout/err pipes are not closed. This is related behavior which context manager would solve.

[giampaolo]

Definitively a good idea. Thanks for your PR but I preferred to do this: 9386585
...so that the ctx manager protocol is available also on Python 2.

[arcivanov]

@giampaolo,

Actually, ctx manager is not expected to be available in 2 since it was only introduced in 3.2 and IMO compliant behavior should be replicated to not create a behavioral inconsistency prior to 3.2.

[arcivanov]

I would also not copy the code to ensure that process context manager behaves exactly like it does in a particular version of Python.

[giampaolo]

I would also not copy the code to ensure that process context manager behaves
exactly like it does in a particular version of Python.

It already works like that. If available (>= 3.2) with 9386585 psutil relies on the exiting __exit__ implementation and I just did the same for __enter__ with 2b162e4.
So really we're relying on the subprocess original implementation, except for Python 2.7 where this represents a backport of the 3.2 functionality.

Actually, ctx manager is not expected to be available in 2 since it was
only introduced in 3.2 and IMO compliant behavior should be replicated
to not create a behavioral inconsistency prior to 3.2.

I don't see anything wrong with backporting this functionality as psutil.Popen represents a wrapper on top of subprocess.Popen.
ŧerminate() and kill(), for example, already take precedence over the original subprocess.Popen() methods (they add the ability to prevent terminating/killing a process which is no longer active and its PID being reused).

[arcivanov]

I'm not arguing it's "wrong" per se, but it's one thing to fix an improper behavior in an existing method, but it's quite another to introduce an API change to the versions where it's not expected in compliant interpreter.

A hypothetical code (however awkward and brittle) may rely on checking whether __enter__ method exists in Popen to determine if Python version is 3.2 or higher, which would be broken by this change. I realize it's an edge of an edge hypothetical, but wouldn't it be safer to simply preserve the API and match user's expectation of behavior for a drop-in replacement?

[giampaolo]

I'm not arguing it's "wrong" per se, but it's one thing to fix an improper behavior in an
existing method, but it's quite another to introduce an API change to the versions
where it's not expected in compliant interpreter.

This may not be expected or improper for subprocess.Popen, in which case I would agree with you, but here we're talking about psutil.Popen, which is a third-party wrapper exposing a tons of additional methods on top of subproces.Popen already.
__enter__ / __exit__ is just yet another addition, and exposing it all the time will save the user the extra hassle deriving from checking the Python version, psutil version or whether __enter__ is defined at runtime.
psutil.Popen makes the promise it can always be used as a ctx manager no matter what the Python version is and that really is it.

IMO it is just more practical if the interface tries to expose the same things on all python versions as this is a principle which is already applied to all other psutil APIs. The fact that psutil.Popen is a wrapper on top of subprocess.Popen is not a good enough reason to break this rule.

To quote the Python Zen I would argue this is a case where practicality beats purity.

I don't expect anybody using hasattr(psutil.Popen, "__enter__") or hasattr(subprocess.Popen, "__enter__") to check the Python version at runtime. I expect they would do that to check whether the object supports the with statement, not the Python version.

[arcivanov]

@giampaolo, ok, I give up, thanks for the fix. 😄