bpo-34663: Add support for POSIX_SPAWN_USEVFORK in posix_spawn when available by pablogsal · Pull Request #9271 · python/cpython

pablogsal · 2018-09-13T18:15:43Z

https://bugs.python.org/issue34663

vstinner · 2018-09-13T19:07:28Z

+        all_flags |= POSIX_SPAWN_SETSCHEDULER;
+#else
+        PyErr_SetString(PyExc_NotImplementedError,
+                "The UseVFork option is not supported in this system.");


The parameter is called "usevfork", the error message should use the same name (not "UseVFork").

vstinner · 2018-09-13T19:07:47Z

        The sigmask to use with the POSIX_SPAWN_SETSIGDEF flag.
    scheduler: object = NULL
        A tuple with the scheduler policy (optional) and parameters.
+    usevfork: bool(accept={int}) = False


I suggest the name "use_vfork", or maybe even "vfork".

bedevere-bot · 2018-09-13T19:08:45Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pablogsal · 2018-09-13T19:19:14Z

I have made the requested changes; please review again

bedevere-bot · 2018-09-13T19:19:17Z

Thanks for making the requested changes!

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

vstinner · 2018-09-13T20:40:17Z

+#ifdef POSIX_SPAWN_USEVFORK
+        all_flags |= POSIX_SPAWN_SETSCHEDULER;
+#else
+        PyErr_SetString(PyExc_NotImplementedError,


You can use argument_unavailable_error() here.

I discussed with Pablo and using "posix_spawn()" function name is less useless than mentioning the "POSIX_SPAWN_USEVFORK flag".

vstinner · 2018-09-13T20:40:50Z

    scheduler: object = NULL
        A tuple with the scheduler policy (optional) and parameters.
+    use_vfork: bool(accept={int}) = False
+        If the value is `True` the POSIX_SPAWN_USEVFORK will be activated.


"If the value is true": use_vfork=1 should be equivalent to use_vfork=True.

vstinner · 2018-09-13T20:41:56Z


+    def test_vfork(self):
+        try:
+            pid = posix.posix_spawn(


You might use args = self.python_args() and posix.posix_spawn(args[0], args, ...)

vstinner · 2018-09-13T20:43:41Z

Please also document the use_vfork parameter in Doc/library/os.html

vstinner · 2018-09-13T21:10:18Z

+  If the value of *use_vfork* is true, the child will be created using
+  :c:func:`vfork` instead of :c:func:`fork`. This corresponds to the
+  GNU specific flag :c:data:`POSIX_SPAWN_USEVFORK`.
+


Oh, "Availability: ..." is missing: this function is not available on Windows, right?

I would suggest to document in "Availability: ..." that the flag is only available on Linux.

vstinner · 2018-09-13T21:10:45Z


+  If the value of *use_vfork* is true, the child will be created using
+  :c:func:`vfork` instead of :c:func:`fork`. This corresponds to the
+  GNU specific flag :c:data:`POSIX_SPAWN_USEVFORK`.


You should document that the function raises an exception if the flag is unavailable on the platform.

vstinner · 2018-09-13T21:11:24Z

+                use_vfork=True
+            )
+        except NotImplementedError:
+            unittest.SkipTest("POSIX_SPAWN_USEVFORK not available on this system")


Hum, you can just reuse the exception message as part of this error message.

vstinner · 2018-09-13T21:12:03Z

@@ -0,0 +1,2 @@
+Added support for `usevfork` parameters of `posix_spawn`, which allows to


use_vfork with underscore, no?

posix_spawn can be written :func:os.posix_spawn to get a link to the function in the generated doc.

bedevere-bot · 2018-09-13T21:12:22Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pablogsal · 2018-09-14T17:22:31Z

I have made the requested changes; please review again

bedevere-bot · 2018-09-14T17:22:34Z

Thanks for making the requested changes!

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

vstinner · 2018-09-14T21:01:20Z

+  GNU specific flag :c:data:`POSIX_SPAWN_USEVFORK`. If the flag is not
+  available on the platform, :exc:`NotImplementedError` will be raised.
+
+   Availability: Using *use_vfork* is only available on Linux.


You should document that the function is only available on Unix. Example:
https://docs.python.org/dev/library/os.html#os.fork

vstinner · 2018-09-14T21:01:41Z

            )
-        except NotImplementedError:
-            unittest.SkipTest("POSIX_SPAWN_USEVFORK not available on this system")
+        except NotImplementedError as e:


nitpick: i prefer to avoid single character variable, i suggest the name "exc".

vstinner · 2018-09-14T21:02:20Z

-        except NotImplementedError:
-            unittest.SkipTest("POSIX_SPAWN_USEVFORK not available on this system")
+        except NotImplementedError as e:
+            raise unittest.SkipTest(e)


You can use: raise self.skipTest(str(exc)).

vstinner · 2018-09-14T21:02:55Z

+#ifdef POSIX_SPAWN_USEVFORK
+        all_flags |= POSIX_SPAWN_SETSCHEDULER;
+#else
+        PyErr_SetString(PyExc_NotImplementedError,


I discussed with Pablo and using "posix_spawn()" function name is less useless than mentioning the "POSIX_SPAWN_USEVFORK flag".

vstinner

LGTM.

@gpshead: Would you mind to double check this PR?

@benjaminp: Would you be interested to have a look as well? (the plan is then to experiment to use it in subprocess for performance.)

pablogsal requested a review from vstinner September 13, 2018 18:15

the-knights-who-say-ni added the CLA signed label Sep 13, 2018

bedevere-bot added the awaiting merge label Sep 13, 2018

Add support for POSIX_SPAWN_USEVFORK in posix_spawn when available

2fe1089

pablogsal force-pushed the usevfork_posix_spawn branch from 404e2d5 to 2fe1089 Compare September 13, 2018 18:17

vstinner requested changes Sep 13, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 13, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 13, 2018

Rename parameter to and correct error

e72c246

pablogsal force-pushed the usevfork_posix_spawn branch from b67b99d to e72c246 Compare September 13, 2018 20:37

vstinner reviewed Sep 13, 2018

View reviewed changes

Add docs and improve implementation and naming

2fe9bf1

pablogsal force-pushed the usevfork_posix_spawn branch from 8bad285 to 2fe9bf1 Compare September 13, 2018 20:57

vstinner requested changes Sep 13, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Sep 13, 2018

pablogsal force-pushed the usevfork_posix_spawn branch 3 times, most recently from 71f1afa to 9c14de2 Compare September 13, 2018 21:38

Improve documentation and error messages

70db5e9

pablogsal force-pushed the usevfork_posix_spawn branch from 9c14de2 to 70db5e9 Compare September 13, 2018 22:01

bedevere-bot removed the awaiting changes label Sep 14, 2018

bedevere-bot added the awaiting change review label Sep 14, 2018

vstinner reviewed Sep 14, 2018

View reviewed changes

pablogsal added 2 commits September 14, 2018 14:20

Add availability and clean tests

6f03dc5

fixup! Add availability and clean tests

c8b46c6

vstinner approved these changes Sep 14, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Sep 14, 2018

pablogsal closed this Sep 20, 2018

pablogsal deleted the usevfork_posix_spawn branch September 20, 2018 10:50

pablogsal restored the usevfork_posix_spawn branch January 9, 2019 18:30

pablogsal deleted the usevfork_posix_spawn branch May 19, 2021 18:57

		@@ -0,0 +1,2 @@
		Added support for `usevfork` parameters of `posix_spawn`, which allows to

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 13, 2018

Uh oh!

pablogsal commented Sep 13, 2018

Uh oh!

bedevere-bot commented Sep 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 13, 2018

Uh oh!

pablogsal commented Sep 14, 2018

Uh oh!

bedevere-bot commented Sep 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pablogsal commented Sep 13, 2018 •

edited by bedevere-bot

Loading