bpo-18299: extending a kill_on_os_error() in Popen #13908

shihai1991 · 2019-06-08T06:54:14Z

Using context manager kill subprocess if a python's exception raised.

https://bugs.python.org/issue18299

Using context manager kill subprocess if a python's exception raised.

asvetlov

Tests for this new feature are required

bedevere-bot · 2019-06-08T07:49:12Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

shihai1991 · 2019-06-08T10:30:57Z

Tests for this new feature are required

Andrew, thanks for review. I will replenish it.

…18299_2

shihai1991 · 2019-06-08T16:35:35Z

Lib/test/test_subprocess.py

@@ -2814,6 +2814,11 @@ def test_stopped(self):

        self.assertEqual(returncode, -3)

+    def test_kill_on_error(self):


Looks it only can add positive testcase due to the core code just only yield self
If somebody have a good idea about negatigve testcase, pls ping me;)

you could create a Popen subclass that overrides either __enter__ or __exit__ to raise an exception. (or do the equivalent using unittest.mock)

Both of them can not work in here. for the reason:

We use contextmanager in the new function and it can not work with __enter__ or __exit__.

Looks like yield proc can not be mocked to raise a error.

shihai1991 · 2019-06-08T16:37:51Z

I have made the requested changes; please review again

bedevere-bot · 2019-06-08T16:37:53Z

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

gpshead

It is not entirely clear to me what this method is intended to do. Its name doesn't make that obvious to me, "on error" is ambiguous.

gpshead · 2019-06-08T22:18:03Z

Doc/library/subprocess.rst

@@ -723,6 +723,11 @@ Instances of the :class:`Popen` class have the following methods:
   On Windows :meth:`kill` is an alias for :meth:`terminate`.


+.. method:: Popen.kill_on_error()
+
+   Kills the child when a python error raised.


When a python error is raised from where and when?

Hi, Gregory, thank for your elaborative review:).
one practical case would be found in https://bugs.python.org/issue25122(due to some race condition on FreeBSD) and i updated the subprocess.rst too.

gpshead · 2019-06-08T22:21:43Z

Lib/subprocess.py

+
+        @contextlib.contextmanager
+        def kill_on_error(self):
+            """Using context manager to kill subprocess if


How does using a context manager kill the process and why would someone reading this understand that to be what it does? Does it matter to the docstring reader if a context manager is used?

similar to the documentation comment: if an python exception is raised is non-specific. raised from where and when? this could easily be misinterpreted to have something to do with the exit status of the child process or even exceptions raised within it.

"on error" is a very ambiguious method name. someone reading code calling this won't intuitively understand what it does other than hopefully kill the process.

Using context manager can use try...except... statement to trap the error. I udpated the docstring too.
I use on_os_error to replace on_error.

gpshead · 2019-06-08T22:25:11Z

Lib/subprocess.py

+                try:
+                    self.kill()
+                except OSError:
+                    # process already terminated


FYI - There is another reason why self.kill() can fail: PermissionError. When a setuid process is launched (or potentially other system configurations enforcing process policies) you may get a PermissionError trying to signal that process.

When a PermissionError happens, the wait() call will block until the process dies on its own, if ever. https://bugs.python.org/issue37091 covers this one. It isn't clear if we should do anything for this situation.

Looks try all possible exceptions or remove all possible exceptions is fine.

gpshead · 2019-06-08T22:26:14Z

Lib/test/test_subprocess.py

@@ -2814,6 +2814,11 @@ def test_stopped(self):

        self.assertEqual(returncode, -3)

+    def test_kill_on_error(self):


you could create a Popen subclass that overrides either __enter__ or __exit__ to raise an exception. (or do the equivalent using unittest.mock)

bedevere-bot · 2019-06-08T22:27:02Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

shihai1991 · 2019-06-09T07:49:35Z

I have made the requested changes; please review again

bedevere-bot · 2019-06-09T07:49:38Z

Thanks for making the requested changes!

@asvetlov, @gpshead: please review the changes made to this pull request.

gpshead · 2019-06-13T03:53:18Z

I'm rejecting this PR. This is not an API we need on the subprocess.Popen class.

There may be utility on a context manager that tries to terminate a process when an exception is escaping the context, but that belongs in its own library. It isn't something users of subprocess need on a regular basis.

The PR references bpo-18299 but I see no relation to that bug. If something like this would've been used in that bug maybe this belongs in test.script_helper.

Misc review notes for posterity here rather than trying to connect them with parts of the diffs:

The description in the documentation still makes no sense. We don't protect anyone from race conditions or hangs. And the method name is still misleading. It appears to attempt to kill the process if any exception escapes the context manager followed by waiting for it regardless of kill succeeding or not (that wait may hang if not). Yet the method name is confusing as a call to Popen.kill() tries to kill the process immediately while a call to Popen.kill_on_os_error() does nothing unless the caller uses it as a context manager. A better name if this were to be a method at all would be something more along the lines of .get_killing_context.

It is possible to write unittests for this functionality, tests would be required.

shihai1991 · 2019-06-13T09:51:18Z

I'm rejecting this PR. This is not an API we need on the subprocess.Popen class.

There may be utility on a context manager that tries to terminate a process when an exception is escaping the context, but that belongs in its own library. It isn't something users of subprocess need on a regular basis.

The PR references bpo-18299 but I see no relation to that bug. If something like this would've been used in that bug maybe this belongs in test.script_helper.

Misc review notes for posterity here rather than trying to connect them with parts of the diffs:

The description in the documentation still makes no sense. We don't protect anyone from race conditions or hangs. And the method name is still misleading. It appears to attempt to kill the process if any exception escapes the context manager followed by waiting for it regardless of kill succeeding or not (that wait may hang if not). Yet the method name is confusing as a call to Popen.kill() tries to kill the process immediately while a call to Popen.kill_on_os_error() does nothing unless the caller uses it as a context manager. A better name if this were to be a method at all would be something more along the lines of .get_killing_context.

It is possible to write unittests for this functionality, tests would be required.

Copy that.

extending subprocess's api

1cddf7e

Using context manager kill subprocess if a python's exception raised.

shihai1991 requested a review from gpshead as a code owner June 8, 2019 06:54

the-knights-who-say-ni added the CLA signed label Jun 8, 2019

bedevere-bot added the awaiting review label Jun 8, 2019

asvetlov requested changes Jun 8, 2019

View reviewed changes

bedevere-bot removed the awaiting review label Jun 8, 2019

bedevere-bot added the awaiting changes label Jun 8, 2019

shihai1991 and others added 4 commits June 9, 2019 00:18

add positive wtest

26da438

📜🤖 Added by blurb_it.

47730a2

update subprocess.rst

181892a

Merge branch 'bpo-18299_2' of github.com:shihai1991/cpython into bpo-…

a4f6643

…18299_2

shihai1991 commented Jun 8, 2019

View reviewed changes

bedevere-bot removed the awaiting changes label Jun 8, 2019

bedevere-bot added the awaiting change review label Jun 8, 2019

shihai1991 changed the title ~~[WIP] bpo-18299: extending a kill_on_error() in Popen~~ bpo-18299: extending a kill_on_error() in Popen Jun 8, 2019

fix whitespace

53032ee

gpshead requested changes Jun 8, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jun 8, 2019

shihai1991 added 2 commits June 9, 2019 15:07

update the new api of subprocess

4975cdc

try catch possible errors

1f1c4c4

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 9, 2019

shihai1991 changed the title ~~bpo-18299: extending a kill_on_error() in Popen~~ bpo-18299: extending a kill_on_os_error() in Popen Jun 11, 2019

fix whitespace

d068d93

gpshead closed this Jun 13, 2019

		@@ -2814,6 +2814,11 @@ def test_stopped(self):

		self.assertEqual(returncode, -3)

		def test_kill_on_error(self):

Uh oh!

bpo-18299: extending a kill_on_os_error() in Popen #13908

bpo-18299: extending a kill_on_os_error() in Popen #13908

Uh oh!

Conversation

shihai1991 commented Jun 8, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 8, 2019

Uh oh!

shihai1991 commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shihai1991 Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpshead Jun 8, 2019

Choose a reason for hiding this comment

Uh oh!

shihai1991 Jun 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihai1991 commented Jun 8, 2019

Uh oh!

bedevere-bot commented Jun 8, 2019

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

gpshead Jun 8, 2019

Choose a reason for hiding this comment

Uh oh!

shihai1991 Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

gpshead Jun 8, 2019

Choose a reason for hiding this comment

Uh oh!

shihai1991 Jun 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpshead Jun 8, 2019

Choose a reason for hiding this comment

Uh oh!

shihai1991 Jun 9, 2019

Choose a reason for hiding this comment

Uh oh!

gpshead Jun 8, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 8, 2019

Uh oh!

shihai1991 commented Jun 9, 2019

Uh oh!

bedevere-bot commented Jun 9, 2019

Uh oh!

gpshead commented Jun 13, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shihai1991 commented Jun 13, 2019

Uh oh!

Uh oh!

shihai1991 commented Jun 8, 2019 •

edited by bedevere-bot

Loading

shihai1991 commented Jun 8, 2019 •

edited

Loading

shihai1991 Jun 8, 2019 •

edited

Loading

shihai1991 Jun 9, 2019 •

edited

Loading

shihai1991 Jun 9, 2019 •

edited

Loading

gpshead commented Jun 13, 2019 •

edited by bedevere-bot

Loading