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

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

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

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

though I see Serhiy diagreeing, follow his lead.

Please regenerate the clinic file.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest a stricter test:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

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



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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this / should be here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made the requested changes; please review again

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

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

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

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.