bpo-20104: Add flag capabilities to posix_spawn #6693

pablogsal · 2018-05-02T10:54:32Z

https://bugs.python.org/issue20104

pablogsal · 2018-05-02T12:42:11Z

@vstinner @serhiy-storchaka Please, review and once we are happy with the interface and implementation we can add new tests for the flags in this PR.

pablogsal · 2018-05-02T13:00:29Z

This implementation is missing setting the POSIX_SPAWN_SETSCHEDPARAM flag implementation because I am not sure what is the best way to actually implement the interface to set the scheduler parameters unsing posix_spawnattr_setschedparam. Any ideas?

pablogsal · 2018-05-05T19:19:11Z

@gpshead @serhiy-storchaka I have implemented the existing flags as keyword-only arguments as suggested in the issue. The question of what to do with POSIX_SPAWN_SETSCHEDPARAM remains.

pablogsal · 2018-05-05T19:23:08Z

Modules/posixmodule.c

@@ -5292,9 +5418,15 @@ os_posix_spawn_impl(PyObject *module, path_t *path, PyObject *argv,
        file_actionsp = &file_actions_buf;
    }

+    if (parse_posix_spawn_flags(setpgroup, resetids, setsigmask,


Notice that in case that non of the flags are set the attr object is still created (with no flags or arguments set). This is to avoid wrapping these lines in

if ((setpgroup != Py_None) | (resetids != Py_True) | (setsigmask != Py_None) | (setsigdef != Py_None) | (setscheduller != Py_None) ) { ... }

As far as I understand there is no different between this and passing NULL to posix_spawn.

pablogsal · 2018-05-06T00:20:23Z

Modules/posixmodule.c

@@ -5114,6 +5114,165 @@ enum posix_spawn_file_actions_identifier {
    POSIX_SPAWN_DUP2
 };

+static int
+iterable_to_sigset(PyObject *iterable, sigset_t *mask)


Notice that this same function exists in Modules/signalmodule.c but is private (static). I did not want to export that symbol in libpython so I recreated the function here. Please, advise if you think a header/extern declaration should be created and the function should be exported (in this case we will need to name it properly).

This is fine. A future refactoring can combine these if ever desired (and if both extension modules are statically linked, a good linker would merge them as being identical anyways).

though I see Serhiy diagreeing, follow his lead.

vstinner · 2018-07-24T13:41:10Z

Lib/test/test_posix.py

+        with self.assertRaises(TypeError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ, resetids=0)


What's the rationale for rejecting 0 as false? resetids=0 seems legit to me.

Why not using "bool" type for this parameter? Something like:

@@ -5416,7 +5411,7 @@ os.posix_spawn * setpgroup: object = NULL The pgroup to use with the POSIX_SPAWN_SETPGROUP flag. - resetids: object = False + resetids: bool = False If the value is `True` the POSIX_SPAWN_RESETIDS will be activated. setsigmask: object(c_default='NULL') = () The sigmask to use with the POSIX_SPAWN_SETSIGMASK flag.

If we use bool then the truthiness value of the object will be used. This will allow the user to pass anything there (ant the boolean value of the argument will be used), right?

I would prefer to pass only True or False (maybe 0 and 1 as well) but not any object.

vstinner · 2018-07-24T13:42:09Z

Lib/test/test_posix.py

+        with self.assertRaises(ValueError):
+            posix.posix_spawn(sys.executable,
+                              [sys.executable, "-c", "pass"],
+                              os.environ, setsigmask=[9998, 9999])


I would prefer a more explicit signal.NSIG rather than arbitrary integer 9999. Maybe [signal.NSIG, signal.NSIG+1]?

vstinner · 2018-07-24T13:45:30Z

Lib/test/test_posix.py

+
+        pid2, status = os.waitpid(pid, 0)
+        self.assertEqual(pid2, pid)
+        self.assertNotEqual(status, 0)


I would suggest a stricter test:

self.assertTrue(os.WIFSIGNALED(status), status) self.assertEqual(os.WTERMSIG(status), signal.SIGUSR1)

bedevere-bot · 2018-07-24T13:46:03Z

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

vstinner · 2018-07-24T13:46:25Z

The overall change LGTM, but I have a few questions/suggestions.

ZackerySpytz · 2018-07-24T19:45:14Z

Doc/library/os.rst

@@ -3392,7 +3392,9 @@ written in Python, such as a mail server's external command delivery program.
   subprocesses.


-.. function:: posix_spawn(path, argv, env, file_actions=None)
+.. function:: posix_spawn(path, argv, env, file_actions=None, /, *, \


I don't believe this / should be here.

pablogsal · 2018-08-31T22:03:13Z

I have made the requested changes; please review again

bedevere-bot · 2018-08-31T22:03:16Z

Thanks for making the requested changes!

@vstinner, @serhiy-storchaka: please review the changes made to this pull request.

vstinner

LGTM.

vstinner · 2018-09-07T15:53:02Z

Yeah! It's now time to see how to use it in subprocess :-)

serhiy-storchaka · 2018-09-08T11:59:08Z

I don't see a use case for it in the stdlib.

vstinner · 2018-09-09T11:24:25Z

I am not talking about the new flags, but more generally, I would like to see if using posix_spawn() rather than _posixsubprocess would make subprocess faster. Yes, according to earlier Pablo's benchmarks.

the-knights-who-say-ni added the CLA signed label May 2, 2018

bedevere-bot added the awaiting review label May 2, 2018

pablogsal force-pushed the complete_posix_spawn branch 3 times, most recently from b838fd6 to eec6215 Compare May 2, 2018 11:07

pablogsal changed the title ~~bpo-20104: Add flag capabilities to posix_spawn~~ bpo-20104: Add flag capabilities to posix_spawn DO NOT MERGE May 2, 2018

pablogsal force-pushed the complete_posix_spawn branch 11 times, most recently from 6946cd3 to 55d1b3d Compare May 2, 2018 18:04

gpshead requested a review from serhiy-storchaka May 2, 2018 21:49

gpshead added the DO-NOT-MERGE label May 4, 2018

pablogsal force-pushed the complete_posix_spawn branch 2 times, most recently from 395e7af to 457ea58 Compare May 5, 2018 19:22

pablogsal commented May 5, 2018

View reviewed changes

pablogsal force-pushed the complete_posix_spawn branch 4 times, most recently from 31bb4bc to 98e005a Compare May 6, 2018 00:18

pablogsal commented May 6, 2018

View reviewed changes

serhiy-storchaka approved these changes Jul 24, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 24, 2018

serhiy-storchaka requested review from gpshead and benjaminp July 24, 2018 11:13

serhiy-storchaka added the type-feature A feature request or enhancement label Jul 24, 2018

vstinner requested changes Jul 24, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jul 24, 2018

ZackerySpytz reviewed Jul 24, 2018

View reviewed changes

vstinner mentioned this pull request Aug 2, 2018

bpo-33613, test_semaphore_tracker_sigint: fix race condition #7850

Merged

Make tests more strict

c784e0f

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 31, 2018

pablogsal added 2 commits September 3, 2018 22:40

Use signal.NSIG for testing invalid signals

514f55e

Accept 1 and 0 as valid values for resetids

abfcd6a

vstinner approved these changes Sep 3, 2018

View reviewed changes

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

pablogsal merged commit 254a466 into python:master Sep 7, 2018

bedevere-bot removed the awaiting merge label Sep 7, 2018

pablogsal deleted the complete_posix_spawn branch September 7, 2018 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-20104: Add flag capabilities to posix_spawn #6693

bpo-20104: Add flag capabilities to posix_spawn #6693

pablogsal commented May 2, 2018 •

edited by bedevere-bot

Loading

pablogsal commented May 2, 2018

pablogsal commented May 2, 2018 •

edited

Loading

pablogsal commented May 5, 2018

pablogsal May 5, 2018 •

edited

Loading

pablogsal May 6, 2018

gpshead May 6, 2018

gpshead May 6, 2018

vstinner Jul 24, 2018

pablogsal Aug 31, 2018

vstinner Jul 24, 2018

vstinner Jul 24, 2018

vstinner Sep 3, 2018

bedevere-bot commented Jul 24, 2018

vstinner commented Jul 24, 2018

ZackerySpytz Jul 24, 2018

serhiy-storchaka Jul 24, 2018

pablogsal commented Aug 31, 2018

bedevere-bot commented Aug 31, 2018

vstinner left a comment

vstinner commented Sep 7, 2018

serhiy-storchaka commented Sep 8, 2018

vstinner commented Sep 9, 2018 via email

bpo-20104: Add flag capabilities to posix_spawn #6693

bpo-20104: Add flag capabilities to posix_spawn #6693

Conversation

pablogsal commented May 2, 2018 • edited by bedevere-bot Loading

pablogsal commented May 2, 2018

pablogsal commented May 2, 2018 • edited Loading

pablogsal commented May 5, 2018

pablogsal May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Jul 24, 2018

vstinner commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablogsal commented Aug 31, 2018

bedevere-bot commented Aug 31, 2018

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Sep 7, 2018

serhiy-storchaka commented Sep 8, 2018

vstinner commented Sep 9, 2018 via email

pablogsal commented May 2, 2018 •

edited by bedevere-bot

Loading

pablogsal commented May 2, 2018 •

edited

Loading

pablogsal May 5, 2018 •

edited

Loading