bpo-35537: Add setsid parameter to os.posix_spawn() and os.posix_spawnp().

[nanjekyejoannah]

I have added a new setsid parameter to os.posix_spawnp() and os.posix_spawn().

I have not yet added a test for this.

@vstinner we can discuss this once you think this patch is good enough.

https://bugs.python.org/issue35537

[vstinner]

Need a test. You can copy and adapt the test_subprocess test:

 def test_start_new_session(self):
# For code coverage of calling setsid(). We don't care if we get an
# EPERM error from it depending on the test execution environment, that
# still indicates that it was called.
try:
output = subprocess.check_output(
[sys.executable, "-c",
"import os; print(os.getpgid(os.getpid()))"],
start_new_session=True)
except OSError as e:
if e.errno != errno.EPERM:
raise
else:
parent_pgid = os.getpgid(os.getpid())
child_pgid = int(output)
self.assertNotEqual(parent_pgid, child_pgid)

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[vstinner]

That's wrong: parse_posix_spawn_flags() is called from py_posix_spawnp(), so function_name should be "posix_spawnp" in that case. You have to add a parameter to parse_posix_spawn_flags().

nitpick: I suggest to rename the variable to "func_name".

[nanjekyejoannah]

@vstinner I made some changes . PTAL.

[izbyshev]

@nanjekyejoannah@vstinner Um, some of my comments were marked as resolved without addressing them, in particular, about shadowing the standard function with setsid parameter and usage of os.getpgid() instead of os.getsid() in the test. I'd prefer to have a discussion if there is an objection. Thanks.

[vstinner]

@nanjekyejoannah@vstinner Um, some of my comments were marked as resolved without addressing them, in particular, about shadowing the standard function with setsid parameter and usage of os.getpgid() instead of os.getsid() in the test. I'd prefer to have a discussion if there is an objection. Thanks.

Oh, I didn't notice that. According to the GitHub UI, I was the only one who "requested changes" (voted -1 on the PR).

I proposed to @nanjekyejoannah to reuse test_start_new_session() from test_subprocess. The comment of this test mentions setsid(). The 'start_new_session' parameter of subprocess.Popen becomes the 'call_setsid' parameter of _posixsubprocess.fork_exec() which then calls setsid(). If os.getpgid() is not appropriate: test_subprocess should be fixed as well.

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Installed with X 3.x has failed when building commit 80c5dfe.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/103/builds/2023) and take a look at the build logs.
4. Check if the failure is related to this commit (80c5dfe) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/103/builds/2023

Click to see traceback logs

From https://github.com/python/cpython
* branch master -> FETCH_HEAD
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
Reset branch 'master'
Python/ast.c: In function ‘handle_keywordonly_args.isra.21’:
Python/ast.c:1415:35: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
arg->type_comment = NEW_TYPE_COMMENT(ch);^Python/ast.c: In function ‘ast_for_arguments’:
Python/ast.c:1619:35: warning: ‘arg’ may be used uninitialized in this function [-Wmaybe-uninitialized]
arg->type_comment = NEW_TYPE_COMMENT(ch);^
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_classdef’:
./Include/Python-ast.h:495:58: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define ClassDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9) _Py_ClassDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
^~~~~~~~~~~~
Python/ast.c:4340:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:495:58: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define ClassDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9) _Py_ClassDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
^~~~~~~~~~~~
Python/ast.c:4340:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_stmt’:
./Include/Python-ast.h:545:49: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define Try(a0, a1, a2, a3, a4, a5, a6, a7, a8) _Py_Try(a0, a1, a2, a3, a4, a5, a6, a7, a8)
^~~~~~~
Python/ast.c:4189:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset, n_except = (nch -3)/3;^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:545:49: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define Try(a0, a1, a2, a3, a4, a5, a6, a7, a8) _Py_Try(a0, a1, a2, a3, a4, a5, a6, a7, a8)
^~~~~~~
Python/ast.c:4189:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset, n_except = (nch -3)/3;^~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:681:55: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define ExceptHandler(a0, a1, a2, a3, a4, a5, a6, a7) _Py_ExceptHandler(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~~~~~~~~~~
Python/ast.c:4128:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:681:55: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define ExceptHandler(a0, a1, a2, a3, a4, a5, a6, a7) _Py_ExceptHandler(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~~~~~~~~~~
Python/ast.c:4128:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:526:47: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define While(a0, a1, a2, a3, a4, a5, a6, a7) _Py_While(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~~
Python/ast.c:4016:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:526:47: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define While(a0, a1, a2, a3, a4, a5, a6, a7) _Py_While(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~~
Python/ast.c:4016:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:530:44: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define If(a0, a1, a2, a3, a4, a5, a6, a7) _Py_If(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~
Python/ast.c:3886:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:530:44: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define If(a0, a1, a2, a3, a4, a5, a6, a7) _Py_If(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~
Python/ast.c:3886:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_funcdef_impl’:
./Include/Python-ast.h:484:66: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define FunctionDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10) _Py_FunctionDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10)
^~~~~~~~~~~~~~~
Python/ast.c:1738:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:484:66: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define FunctionDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10) _Py_FunctionDef(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10)
^~~~~~~~~~~~~~~
Python/ast.c:1738:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_for_stmt’:
./Include/Python-ast.h:518:53: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define For(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9) _Py_For(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
^~~~~~~
Python/ast.c:4065:21: note: ‘end_col_offset’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:518:53: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define For(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9) _Py_For(a0, a1, a2, a3, a4, a5, a6, a7, a8, a9)
^~~~~~~
Python/ast.c:4065:9: note: ‘end_lineno’ was declared here
int end_lineno, end_col_offset;^~~~~~~~~~
In file included from Python/ast.c:7:0:
Python/ast.c: In function ‘ast_for_with_stmt’:
./Include/Python-ast.h:534:46: warning: ‘end_col_offset’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define With(a0, a1, a2, a3, a4, a5, a6, a7) _Py_With(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~
Python/ast.c:4292:67: note: ‘end_col_offset’ was declared here
int i, n_items, nch_minus_type, has_type_comment, end_lineno, end_col_offset;^~~~~~~~~~~~~~
In file included from Python/ast.c:7:0:
./Include/Python-ast.h:534:46: warning: ‘end_lineno’ may be used uninitialized in this function [-Wmaybe-uninitialized]
#define With(a0, a1, a2, a3, a4, a5, a6, a7) _Py_With(a0, a1, a2, a3, a4, a5, a6, a7)
^~~~~~~~
Python/ast.c:4292:55: note: ‘end_lineno’ was declared here
int i, n_items, nch_minus_type, has_type_comment, end_lineno, end_col_offset;^~~~~~~~~~Python/pystate.c: In function ‘_long_shared’:
Python/pystate.c:1483:18: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
data->data = (void *)value;^Python/symtable.c: In function ‘PySymtable_BuildObject’:
Python/symtable.c:289:5: warning: enumeration value ‘FunctionType_kind’ not handled in switch [-Wswitch]
switch (mod->kind) {
^~~~~~./Modules/posixmodule.c: In function ‘parse_posix_spawn_flags’:
./Modules/posixmodule.c:5173:17: warning: unused variable ‘func_name’ [-Wunused-variable]
const char *func_name ="posix_spawnp";^~~~~~~~~
/buildbot/tmp/tmpdir/tmp4xs5o00b/pip-18.1-py2.py3-none-any.whl/pip/_vendor/requests/status_codes.py:3: SyntaxWarning: invalid escape sequence \o
/buildbot/tmp/tmpdir/tmp4xs5o00b/pip-18.1-py2.py3-none-any.whl/pip/_vendor/requests/status_codes.py:3: SyntaxWarning: invalid escape sequence \o

[vstinner]

Thanks and congrats @nanjekyejoannah, I merged your PR!

I saw that almost all my requests have been addressed exception of 'func_name': you added func_name but its value is hardcoded to "posix_spawnp". I started to write a fix, but then I saw other minor issues in error messages. So I wrote PR #11719 directly instead. I'm fine with working step by step, not having the perfect commit directly.

Sadly, when I testing my "func_name" change, I noticed that "test_posix" fails locally. Oh. Why Travis CI didn't detect it? Oh. Now I recall that Travis CI lacks the required constant and so the test is skipped (NotImplementedError...).

I wrote PR #11718 just to skip the test, to have more time to fix the test.

This PR was awaiting for 2 weeks and got many reviews. I chose to merge it to make progress ;-)

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu Shared 3.x has failed when building commit 80c5dfe.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/141/builds/1142) and take a look at the build logs.
4. Check if the failure is related to this commit (80c5dfe) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/141/builds/1142

Click to see traceback logs

Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 1640, in test_start_new_session
child_pgid =int(output)
NameError: name 'output' is not defined
======================================================================
ERROR: test_start_new_session (test.test_posix.TestPosixSpawnP)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 1640, in test_start_new_session
child_pgid =int(output)
NameError: name 'output' is not defined
----------------------------------------------------------------------
Ran 139 tests in 4.228s
FAILED (errors=2, skipped=12)
Warning -- reap_children() reaped child process 2567
Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 1640, in test_start_new_session
child_pgid =int(output)
NameError: name 'output' is not defined
======================================================================
ERROR: test_start_new_session (test.test_posix.TestPosixSpawnP)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_posix.py", line 1640, in test_start_new_session
child_pgid =int(output)
NameError: name 'output' is not defined
----------------------------------------------------------------------
Ran 139 tests in 1.949s
FAILED (errors=2, skipped=12)

[izbyshev]

Oh, I didn't notice that. According to the GitHub UI, I was the only one who "requested changes" (voted -1 on the PR).

Ah, I see. I prefer to leave simple comments so that they are not mixed with reviews by core devs, but my approach backfired in this case.

If os.getpgid() is not appropriate: test_subprocess should be fixed as well.

While I think that technically the existing os.getpgid check works (because if the session is changed, the process group must be changed as well), os.posix_spawn also has setpgroup parameter, so it's kind of confusing to use the same check in both cases. (Note that the existing test for setpgroup doesn't really check the process group at all).

[pablogsal]

@vstinner I opened https://bugs.python.org/issue35876 after seeing the buildbot failures to track the failing test fixes.