bpo-34663: Add support for POSIX_SPAWN_USEVFORK in posix_spawn when available by pablogsal · Pull Request #9271 · python/cpython
| all_flags |= POSIX_SPAWN_SETSCHEDULER; | ||
| #else | ||
| PyErr_SetString(PyExc_NotImplementedError, | ||
| "The UseVFork option is not supported in this system."); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter is called "usevfork", the error message should use the same name (not "UseVFork").
| 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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest the name "use_vfork", or maybe even "vfork".
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
I have made the requested changes; please review again
Thanks for making the requested changes!
@vstinner: please review the changes made to this pull request.
| #ifdef POSIX_SPAWN_USEVFORK | ||
| all_flags |= POSIX_SPAWN_SETSCHEDULER; | ||
| #else | ||
| PyErr_SetString(PyExc_NotImplementedError, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use argument_unavailable_error() here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed with Pablo and using "posix_spawn()" function name is less useless than mentioning the "POSIX_SPAWN_USEVFORK flag".
| 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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If the value is true": use_vfork=1 should be equivalent to use_vfork=True.
|
|
||
| def test_vfork(self): | ||
| try: | ||
| pid = posix.posix_spawn( |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might use args = self.python_args() and posix.posix_spawn(args[0], args, ...)
Please also document the use_vfork parameter in Doc/library/os.html
| 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`. | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
|
||
| 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`. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should document that the function raises an exception if the flag is unavailable on the platform.
| use_vfork=True | ||
| ) | ||
| except NotImplementedError: | ||
| unittest.SkipTest("POSIX_SPAWN_USEVFORK not available on this system") |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, you can just reuse the exception message as part of this error message.
| @@ -0,0 +1,2 @@ | |||
| Added support for `usevfork` parameters of `posix_spawn`, which allows to | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
I have made the requested changes; please review again
Thanks for making the requested changes!
@vstinner: please review the changes made to this pull request.
| 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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ) | ||
| except NotImplementedError: | ||
| unittest.SkipTest("POSIX_SPAWN_USEVFORK not available on this system") | ||
| except NotImplementedError as e: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: i prefer to avoid single character variable, i suggest the name "exc".
| except NotImplementedError: | ||
| unittest.SkipTest("POSIX_SPAWN_USEVFORK not available on this system") | ||
| except NotImplementedError as e: | ||
| raise unittest.SkipTest(e) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use: raise self.skipTest(str(exc)).
| #ifdef POSIX_SPAWN_USEVFORK | ||
| all_flags |= POSIX_SPAWN_SETSCHEDULER; | ||
| #else | ||
| PyErr_SetString(PyExc_NotImplementedError, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed with Pablo and using "posix_spawn()" function name is less useless than mentioning the "POSIX_SPAWN_USEVFORK flag".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)