bpo-20104: Add flag capabilities to posix_spawn by pablogsal · Pull Request #6693 · python/cpython

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

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

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.

serhiy-storchaka · 2018-06-15T16:07:15Z

Please regenerate the clinic file.

pablogsal · 2018-06-15T16:35:06Z

@serhiy-storchaka I had to rebase onto the latest master to pick up the latest changes and solve the merge conflict of the clinic file. I have done that and regenerated it.

serhiy-storchaka · 2018-07-15T17:33:29Z

Please merge with the master again and regenerate the clinic file.

…and priority

pablogsal · 2018-07-24T11:06:30Z

@serhiy-storchaka Done!

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

+        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

+        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

+
+        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



-.. 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

pablogsal force-pushed the complete_posix_spawn branch from f0bd16d to dcf9ffc Compare May 14, 2018 13:17

pablogsal force-pushed the complete_posix_spawn branch from dcf9ffc to e605b01 Compare June 15, 2018 16:33

pablogsal added 7 commits July 20, 2018 10:52

Add attr capabilities to posix_spawn

4982abd

Add documentation for the new parameters of posix_spawn

ad862cc

Refactor and clean tests for posix_spawn flags

552b1dc

Refactor and clean implementation of parse_posix_spawn_flags

713d803

Fix scheduler test for posix_spawn flags to use correctly the policy …

ecd64e4

…and priority

Indent string code blocks

88a831c

Fix typo in the documentation

0fbae92

pablogsal force-pushed the complete_posix_spawn branch from e605b01 to 8fb0389 Compare July 20, 2018 09:57

Regenerated the clinic files

06262a5

serhiy-storchaka approved these changes Jul 24, 2018

View reviewed changes

vstinner requested changes Jul 24, 2018

View reviewed changes

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

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



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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

pablogsal commented May 2, 2018

Uh oh!

pablogsal commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented May 5, 2018

Uh oh!

pablogsal May 5, 2018 • 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!

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!

serhiy-storchaka commented Jun 15, 2018

Uh oh!

pablogsal commented Jun 15, 2018

Uh oh!

serhiy-storchaka commented Jul 15, 2018

Uh oh!

pablogsal commented Jul 24, 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!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 24, 2018

Uh oh!

vstinner commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Aug 31, 2018

Uh oh!

bedevere-bot commented Aug 31, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 7, 2018

Uh oh!

serhiy-storchaka commented Sep 8, 2018

Uh oh!

vstinner commented Sep 9, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

pablogsal commented May 2, 2018 •

edited by bedevere-bot

Loading

pablogsal commented May 2, 2018 •

edited

Loading

pablogsal May 5, 2018 •

edited

Loading