Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-35537: Document posix_spawn() change in subprocess #11668

Merged
merged 1 commit into from Apr 25, 2019
Merged

bpo-35537: Document posix_spawn() change in subprocess #11668

merged 1 commit into from Apr 25, 2019

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jan 24, 2019

Document that subprocess.Popen no longer raise an exception on error
like missing program on very specific platforms when using
os.posix_spawn() is used.

https://bugs.python.org/issue35537

@@ -567,6 +567,13 @@ functions.
Popen destructor now emits a :exc:`ResourceWarning` warning if the child
process is still running.

.. versionchanged:: 3.8
Popen can now use :func:`os.posix_spawn` in some cases for better
Copy link
Contributor

Choose a reason for hiding this comment

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

What about if we say: Now, Popen can use ... ```` or just remove the now``` .

Just a suggestion

@@ -567,6 +567,13 @@ functions.
Popen destructor now emits a :exc:`ResourceWarning` warning if the child
process is still running.

.. versionchanged:: 3.8
Popen can now use :func:`os.posix_spawn` in some cases for better
performance. On some very specific platforms, Popen constructor using
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you mention some example of that platforms?

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to not mention WSL and QEMU User Emulation since it's a bug and these bugs can be fixed whenever. I prefer to not mention them to be more future-proof (not have to update the doc).

Copy link
Member

Choose a reason for hiding this comment

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

I'd mention the platforms anyways with a "at the time of this writing, buggy platforms we are aware of include Windows Subsystem for Linux (version XYZ) and QEMU (version XYZ)" ... If there are bugreports filed against them, those can be linked to from the doc as well.

Otherwise it such text sets off alarm bells for anyone reading it, they'll have the same question: am I impacted? do I use any "very specific platforms"? and come seeking more information.

when we become aware of upstream bugs being fixed we can update the text to say "versions prior to ABC" or similar.

@vstinner
Copy link
Member Author

@izbyshev: Would you mind to review this change? Are you fine with documentating the limitation rather trying to fixing it?

@izbyshev
Copy link
Contributor

izbyshev commented Jan 25, 2019

@vstinner I hope that problems with QEMU, WSL and any other Linux environments where vfork is emulated via fork will be avoided if bpo-35823 is accepted. If that happens, there will be no known platform that both (a) has the problem you describe and (b) is eligible for posix_spawn as currently implemented in subprocess. Therefore, this change will effectively be the statement that we're OK with using posix_spawn in the future on platforms that suffer from such problems. Or, to put it another way, that we're willing to take the risk if we can't reliably check whether such issues exist when we decide to use posix_spawn. Is that what you intend to do?

Personally, I'd wait at least until we're done with bpo-35823. If it's merged, I'd prefer not to merge a part about "very specific platforms" and wait until we encounter another such case. If it's rejected, I agree that we should document this issue. Also, we could add a parameter to subprocess.Popen() to allow users to force traditional fork()/exec() path to workaround such issues.

@vstinner
Copy link
Member Author

vstinner commented Jan 25, 2019

Personally, I'd wait at least until we're done with bpo-35823.

Ok, let's do that ;-)

@vstinner vstinner changed the title bpo-35537: Document posix_spawn() change in subprocess [WIP] bpo-35537: Document posix_spawn() change in subprocess Jan 25, 2019
@vstinner
Copy link
Member Author

@izbyshev: More generally, I like how things are going one. Slowly, we listed more and more bugs and corner cases, and we had to experiment posix_spawn() and dig into the code (well, in practice that part mostly done by you @izbyshev :-D) to be able to get this list. I'm fine with dropping posix_spawn() from subprocess if we can successfully use vfork in _posixsubprocess. I'm open to different options, as soon as subprocess runs faster :-)

Document that subprocess.Popen no longer raise an exception on error
like missing program on very specific platforms when using
os.posix_spawn() is used.
@vstinner vstinner changed the title [WIP] bpo-35537: Document posix_spawn() change in subprocess bpo-35537: Document posix_spawn() change in subprocess Apr 16, 2019
@vstinner
Copy link
Member Author

Four months later, I see no activity on the idea of modiying _posixsubprocess to use vfork(), so I updated this change and I propose to merge it.

@eamanu and @gpshead: Would you mind to review my update PR?

@vstinner vstinner merged commit d7befad into python:master Apr 25, 2019
@vstinner vstinner deleted the subprocess_spawn_note branch April 25, 2019 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants