[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: Serhiy: > And os.posix_spawnp() still is not implemented. I created bpo-35674 to see how to expose this feature. -- ___ Python tracker ___

[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: I close the issue, it's now done. If someone wants to experiment in posix_spawn() in subprocess, please open a new issue. -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python

[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: > Thank you for your contribution Pablo. This issue have appeared much more complex and less obvious than it looked initially. Yeah, thanks Pablo for your tenacy! This function is interesting in term of performance and correctness ("atomic" function, signal

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: Thank you for your contribution Pablo. This issue have appeared much more complex and less obvious than it looked initially. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: New changeset d700f97b627989d41cd4629dc02969f9a6b56d2f by Serhiy Storchaka in branch 'master': bpo-20104: Change the file_actions parameter of os.posix_spawn(). (GH-6725) https://github.com/python/cpython/commit/d700f97b627989d41cd4629dc02969f9a6b56d2f

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: New changeset 254a4663d8c5970ae2928185c50ebaa6c7e62c80 by Pablo Galindo in branch 'master': bpo-20104: Add flag capabilities to posix_spawn (GH-6693) https://github.com/python/cpython/commit/254a4663d8c5970ae2928185c50ebaa6c7e62c80 --

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: Benjamin, Gregory, could you please look at PR 6693? Is it what you want? -- ___ Python tracker ___

[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: Tests fail on PPC64 Fedora 3.x: bpo-33630. -- ___ Python tracker ___

[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: > I originally removed it from the configure script in PR6794 but it was > reintroduced in commit 57009526f6a405e0ffe8c16012cce509b62cb577. Check the PR > for Greg's rationale. Oh ok. -- ___

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: I originally removed it from the configure script in PR6794 but it was reintroduced in commit 57009526f6a405e0ffe8c16012cce509b62cb577. Check the PR for Greg's rationale. -- ___ Python

[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: posix_spawn can still be found in Python 3.7: configure:11243: posix_fallocate posix_fadvise posix_spawn pread preadv preadv2 \ configure.ac:3470: posix_fallocate posix_fadvise posix_spawn pread preadv preadv2 \ pyconfig.h.in:710:/*

[issue20104] expose posix_spawn(p)

Change by Ned Deily : -- priority: release blocker -> versions: -Python 3.7 ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: Also this reading may be relevant/interesting: https://about.gitlab.com/2018/01/23/how-a-fix-in-go-19-sped-up-our-gitaly-service-by-30x/ -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: Some interesting benchmarks of posix spawn: https://github.com/rtomayko/posix-spawn/blob/master/README.md -- components: -Library (Lib) priority: normal -> release blocker versions: +Python 3.7

[issue20104] expose posix_spawn(p)

Change by Gregory P. Smith : -- components: +Library (Lib) ___ Python tracker ___ ___

[issue20104] expose posix_spawn(p)

Change by Gregory P. Smith : -- priority: release blocker -> normal versions: -Python 3.7 ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: New changeset 8e633a4035bcff458c45fa095f4b8eab2f158466 by Gregory P. Smith (Pablo Galindo) in branch '3.7': bpo-20104: Remove posix_spawn from 3.7 (GH-6794) https://github.com/python/cpython/commit/8e633a4035bcff458c45fa095f4b8eab2f158466

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: Pablo, Victor, Ned and I synced up at the pycon2018 sprints. We're removing posix_spawn from 3.7 (https://github.com/python/cpython/pull/6794) while the API is worked on. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Change by Pablo Galindo Salgado : -- Removed message: https://bugs.python.org/msg316526 ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: Regarding the rationale for when posix_spawn can be useful (from the RATIONALE section of the man page): The posix_spawn() function and its close relation posix_spawnp() have been introduced to overcome the following perceived

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: Notice that https://github.com/python/cpython/pull/6794 is already open to remove posix_spawn from 3.7. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: As for the scheduler interface, yet one option is using two mutually exclusive parameters setschedparam and setscheduler. The first take a sched_param, the second takes a pair: int and sched_param. This will not simplify the

[issue20104] expose posix_spawn(p)

Change by Pablo Galindo Salgado : -- pull_requests: +6479 ___ Python tracker ___ ___

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: I think that is the biggest argument towards using a tuple: that just setting the priority is not enough (and also is decontextualized as different policies have different priorities). On the other hand one could say that the API

[issue20104] expose posix_spawn(p)

Martin Panter added the comment: I suggested the “scheduler” tuple to bring the two related parameters (scheduling policy and sched_param) together, similar to how they are paired as the second and third parameters to “os.sched_setscheduler”, and because I thought it

[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: For Python 3.7, I discussed with Pablo and he seems to be ok to remove the function from Python 3.7, to get more time to polish the API in the master branch (future Python 3.8). Can someone please write a PR to remove the function from

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: I propose the following changes to the file_actions parameter in PR 6725. Currently it is the fourth positional-only parameter with the default value None. 1. Change its default value to an empty tuple. None will no longer be

[issue20104] expose posix_spawn(p)

Change by Serhiy Storchaka : -- pull_requests: +6418 ___ Python tracker ___ ___

[issue20104] expose posix_spawn(p)

Change by Serhiy Storchaka : -- dependencies: +Expose the sigset_t converter via private API ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: I have doubts about combining setschedpolicy and setschedparam. On one hand, they are set by independent functions in C and have separate flags. When pass arguments separately we don't need to invent a parameter name: just

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: Thanks! I have updated the PR and added tests. -- ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Martin Panter added the comment: Can you use the existing sched_param class? https://docs.python.org/3/library/os.html#os.sched_param -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: One open question is how to construct and pass through the struct "sched_param" that “posix_spawnattr_setschedparam” needs. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Martin Panter added the comment: To wrap “posix_spawnattr_setschedparam” perhaps you could combine it with the scheduler policy: # Inherit current policy and parameters: posix_spawn(..., scheduler=None) # Set new policy with parameters: posix_spawn(...,

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: Since this is a new API, the most important thing is to have decided how we want it to look in 3.7, it is okay for it to be missing features so long as the API doesn't prevent them from being added in the future. ie: It is fine to ship it

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: As for PR 6693: I think using keyword arguments would be more Pythonic. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: I have open https://github.com/python/cpython/pull/6693 to start iterating over the missing capabilities of posix_spawn (Passing various attributes of the created child process). Please, review to make sure that the design is OK

[issue20104] expose posix_spawn(p)

Change by Pablo Galindo Salgado : -- pull_requests: +6386 ___ Python tracker ___ ___

[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: > The current implementation looks half-baked to me. (...) I would prefer to see a full implementation before Python 3.7 final, or to remove the feature from Python 3.7 and redo it in Python 3.8. It seems doable to finish the

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: The current implementation looks half-baked to me. It doesn't implement the following features: 1. posix_spawnp(). It is like posix_spawn(), but searches an executable in PATH. 2. Passing various attributes of the created child

[issue20104] expose posix_spawn(p)

Ned Deily added the comment: Thanks for the latest fixes, Serhiy. Now is there anything more that needs to be done for this issue? -- priority: release blocker -> deferred blocker versions: +Python 3.8 ___ Python tracker

[issue20104] expose posix_spawn(p)

miss-islington added the comment: New changeset 77fa7835da0cb49d30ac5d4c32bf6eb71eae0742 by Miss Islington (bot) in branch '3.7': bpo-20104: Improve error handling and fix a reference leak in os.posix_spawn(). (GH-6332)

[issue20104] expose posix_spawn(p)

Change by miss-islington : -- pull_requests: +6368 ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: New changeset ef347535f289baad22c0601e12a36b2dcd155c3a by Serhiy Storchaka in branch 'master': bpo-20104: Improve error handling and fix a reference leak in os.posix_spawn(). (#6332)

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: What about posix_spawnp()? The title mentions both functions, but I don't see any mentions about posix_spawnp() in the discussion. Its omission looks unintentional to me. -- ___

[issue20104] expose posix_spawn(p)

Ned Deily added the comment: Thanks for adding the doc updates. There are a few rendering issues: https://docs.python.org/3.7/library/os.html#os.posix_spawn -- ___ Python tracker

[issue20104] expose posix_spawn(p)

miss-islington added the comment: New changeset ab8457232121dfdfb1d4bfcf806a842fbe402722 by Miss Islington (bot) in branch '3.7': bpo-20104: Add os.posix_spawn documentation. (GH-6334)

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: PR 6332 is overall a good cleanup. I pushed a couple changes to that PR and left comments for some others. It should go in. There are basic tests for os.posix_spawn in test_posix.py. More would be nice as we don't currently have any for

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: New changeset 79760ed256987cead08d668b95675afba6b3760f by Gregory P. Smith in branch 'master': bpo-20104: Add os.posix_spawn documentation. (#6334) https://github.com/python/cpython/commit/79760ed256987cead08d668b95675afba6b3760f

[issue20104] expose posix_spawn(p)

Change by miss-islington : -- pull_requests: +6047 ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Change by Gregory P. Smith : -- pull_requests: +6046 ___ Python tracker ___ ___

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: PR 6332 fixes issues found by me in os.posix_spawn(). It is not completed yet, needed documentation and tests. And I have doubts about API. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Change by Serhiy Storchaka : -- pull_requests: +6045 stage: commit review -> patch review ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: Notice that some of the errors identified at the end of 5109 were already corrected by the other PRs in this issue. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: In PR6331 I am trying to address all issues identified by Serhiy. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Ned Deily added the comment: > perhaps it would be better to remove this undocumented feature from 3.7 We need to make a decision about this before the b4 cutoff in 4 weeks. But it would not be fair to ask people to put in the effort to try to fix things if there is not a

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: I'm trying to fix issues with os.posix_spawn(), but there are so much of them, that perhaps it would be better to remove this undocumented feature from 3.7. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Serhiy Storchaka added the comment: posix_spawn() is not documented, it contains reference leaks (issue33191) and can cause crashes. It's style doesn't conform PEP 7. The simplest fix requires changing the undocumented interface. -- dependencies: +Refleak

[issue20104] expose posix_spawn(p)

Change by Gregory P. Smith : -- assignee: -> gregory.p.smith ___ Python tracker ___ ___

[issue20104] expose posix_spawn(p)

Change by Gregory P. Smith : -- resolution: -> fixed stage: patch review -> commit review status: open -> closed ___ Python tracker

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: New changeset 0cd6bca65519109a8a7862d38ba1b8924e432a16 by Gregory P. Smith (Pablo Galindo) in branch 'master': bpo-20104: Fix leaks and errors in new os.posix_spawn (GH-5418)

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: Thank Yury for the information! This should be fixed now in PR418. -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Change by Yury Selivanov : -- nosy: -yselivanov ___ Python tracker ___ ___

[issue20104] expose posix_spawn(p)

Yury Selivanov added the comment: FYI New defect(s) Reported-by: Coverity Scan Showing 2 of 2 defect(s) ** CID 1428733: Memory - illegal accesses (RETURN_LOCAL) /Modules/posixmodule.c: 5281 in os_posix_spawn_impl()

[issue20104] expose posix_spawn(p)

Change by Ned Deily : -- nosy: +ned.deily priority: normal -> release blocker ___ Python tracker ___

[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: FYI bolen-dmg-3.x was also broken by this change: http://buildbot.python.org/all/#/builders/69/builds/114 -- nosy: +vstinner ___ Python tracker

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: @vstinner I have removed the #define HAVE_POSIX_SPAWN 1 in PR 5418. -- nosy: -vstinner ___ Python tracker

[issue20104] expose posix_spawn(p)

STINNER Victor added the comment: Python master doesn't build on macOS Tiger anymore: http://buildbot.python.org/all/#/builders/30/builds/581 ./Modules/posixmodule.c:251:19: error: spawn.h: No such file or directory ./Modules/posixmodule.c: In function

[issue20104] expose posix_spawn(p)

Martin Panter added the comment: Your assumption about calling “file_actions_destroy” would be okay if the posix_spawn_file_actions_t object was a simple object or structure. But I imagine most implementations would allocate memory when you call one of the “add”

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: Regarding the leak, I was under the assumption that as File_actionsp is pointing to a stack initialized _file_actions and is this last variable the one that is passed to posix_spawn_file_actions_init, it was not needed to

[issue20104] expose posix_spawn(p)

Martin Panter added the comment: TypeError if “posix_spawn_file_actions_init” fails doesn’t seem right. I suggest OSError, MemoryError, or even plain Exception instead. “File_actionsp” is set to point to a local variable “_file_actions”, but the variable goes out of

[issue20104] expose posix_spawn(p)

Martin Panter added the comment: Does the PySequence_Fast result need releasing if the following “for” loop fails? There is a Py_DECREF only in the successful case, which seems inconsistent. Does Python still support non-UTF-8 locales and bytes filenames? I haven’t been

[issue20104] expose posix_spawn(p)

Pablo Galindo Salgado added the comment: I have opened a PR to address this issues: https://github.com/python/cpython/pull/5415 -- ___ Python tracker

[issue20104] expose posix_spawn(p)

Change by Pablo Galindo Salgado : -- keywords: +patch pull_requests: +5253 ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Martin Panter added the comment: Pablo’s code looked unfinished to me. As well as missing documentation, I suspect there may be memory leaks and poor error handling. The two calls above the “fail:” label look like dead code. The “parse_envlist” result appears to be

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: remaining work before closing this: Doc/library/os.rst needs to describe os.posix_spawn -- priority: low -> normal ___ Python tracker

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: New changeset 6c6ddf97c402709713d668d0ed53836a7749ba99 by Gregory P. Smith (Pablo Galindo) in branch 'master': bpo-20104: Expose `posix_spawn` in the os module (GH-5109)

[issue20104] expose posix_spawn(p)

Change by Gregory P. Smith : -- resolution: rejected -> stage: needs patch -> patch review status: closed -> open type: performance -> enhancement ___ Python tracker

[issue20104] expose posix_spawn(p)

Change by Pablo Galindo Salgado : -- pull_requests: +4976 ___ Python tracker ___ ___

[issue20104] expose posix_spawn(p)

Change by Pablo Galindo Salgado : -- pull_requests: +4976, 4977 ___ Python tracker ___

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: All I'm really saying is that someone who wants this should provide a patch/PR with unittests. :) I can help review and go from there. It does make sense to me for it to be available as part of the subprocess API if it is available at all, likely an

[issue20104] expose posix_spawn(p)

John Jones added the comment: I agree with everything you're saying Gregory, however I don't think the significance of the memory doubling is as inconsequential as you might first think. For example, i have on my 64bit Linux system 128Gb of RAM, and a numpy table that's around 70Gb. Spawning

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: I think someone wanting this will need to put forward a patch adding it to be reviewed and mulled over. As Alex mentioned in msg22571 - https://github.com/dreid/posix_spawn/ exists as does the code Danek pointed at in the next comment. try those. I

[issue20104] expose posix_spawn(p)

John Jones added the comment: To prevent subprocess/os.fork() doubling my memory after loading a large numpy array into memory, I now have to start my script with 650 calls to subprocess.Popen(), which just sit their waiting for some stdin to start doing something. This doesn't happen until

[issue20104] expose posix_spawn(p)

Danek Duvall added the comment: Oh, for what it's worth, Solaris added setsid support to posix_spawn a few years ago, as a result of this conversation. I still think it would be worthwhile supporting this in the stdlib, since we keep running into processes which have a lot of memory reserved

[issue20104] expose posix_spawn(p)

Danek Duvall added the comment: Our project (the Solaris packaging system, IPS), relies on posix_spawn() primarily for the ability to fork without making a large memory reservation (and possibly failing) because the forking process was itself very large. That's the (a?) bug benefit of

[issue20104] expose posix_spawn(p)

Alex Gaynor added the comment: Danek, you might find https://github.com/dreid/posix_spawn/ useful, it provides bindings and a public API over posix_spawn (it's not complete yet, but if there's stuff missing, feel free to file a ticket!) -- ___

[issue20104] expose posix_spawn(p)

Danek Duvall added the comment: Cool. We implemented our own version as a straight-up native module (https://java.net/projects/ips/sources/pkg-gate/content/src/modules/pspawn.c), and our Popen replacement is not at present a complete replacement for the one in the stdlib, but it does what we

[issue20104] expose posix_spawn(p)

Benjamin Peterson added the comment: Okay, this seems like a bad idea. -- resolution: - rejected status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20104 ___

[issue20104] expose posix_spawn(p)

New submission from Benjamin Peterson: posix_spawn is a nice, efficient replacement for fork()/exec(). We should expose it and possibly use it in subprocess where possible. -- components: Extension Modules messages: 207137 nosy: benjamin.peterson priority: normal severity: normal

[issue20104] expose posix_spawn(p)

Changes by Gennadiy Zlobin gennad.zlo...@gmail.com: -- nosy: +gennad ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20104 ___ ___ Python-bugs-list

[issue20104] expose posix_spawn(p)

Changes by Alex Gaynor alex.gay...@gmail.com: -- nosy: +alex ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20104 ___ ___ Python-bugs-list mailing

[issue20104] expose posix_spawn(p)

Changes by Antoine Pitrou pit...@free.fr: -- nosy: +gregory.p.smith, neologix ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue20104 ___ ___

[issue20104] expose posix_spawn(p)

Gregory P. Smith added the comment: Unless it could replace the fork+exec code path in its entirety, which I do not believe is possible, I see posix_spawn() as a distraction and additional maintenance burden with no benefit.