#12068: Fix incorrect env handling in spawnProcess on Posix when posix_spawnp is used.

[p12tic]

Fixes #12068. This is a regression starting at Twisted 23.8.0.

Documentation on reactor.spawnProcess says the following about env parameter:

env is None: On POSIX: pass os.environ

This was not correctly handled in os.posix_spawnp() code path.

Scope and purpose

This PR is just a simple one line fix to address #12068. Tests for environment passing on Posix have been added.

[p12tic]

please review

[adiroiban]

Thanks for the PR!

Not sure what to do here.

I think that we should fix the documentation instead.

That is, the bug lies in the documentation and not in the code :)

This change was introduced 2 years ago and is present in a public release.

By doing this change, we will break backward compatibility.

The initial change from 16 years ago was using os.execvpe

https://github.com/twisted/twisted/blame/dee676b040dd38b847ea6fb112a712cb5e119490/src/twisted/internet/process.py#L468

My understanding is that execvpe always requires an explicit environment, and it will not copy the environment from the parent process.

Regards

[glyph]

Not sure what to do here.

I think that we should fix the documentation instead.

That is, the bug lies in the documentation and not in the code :)

This change was introduced 2 years ago and is present in a public release.

By doing this change, we will break backward compatibility.

The initial change from 16 years ago was using os.execvpe

Normally I would agree, except the circumstances under which posix_spawn is used are not entirely under control of the user, and this introduces an inconsistency within twisted's behaviors.

In this specific case, I think the least impactful thing is to fix the code, rather than the docs.

But perhaps we should shoot this over to the mailing list for a tiebreaker?

[p12tic]

This change was introduced 2 years ago and is present in a public release.

This change of behavior was actually released in 23.8.0 by enabling use of posix_spawnp. From release notes: "reactor.spawnProcess() now uses posix_spawnp when possible, making it much more efficient (#5710)"

The regression is actually pretty bad. It silently drops environment variables only under certain conditions (e.g. when path argument of spawnProcess is not specified). Which means that most of the uses of spawnProcess in a certain codebase will work, except a few which depend on environment variables and satisfy the conditions for using posix_spawnp. Additionally, spawnProcess is something that's usually mocked in unit tests. Which means that unit tests wouldn't catch the failure. Only full end to end or manual testing could possibly catch it. These naturally have much worse code coverage, which means that failures are likely to be unnoticed for quite some time.

This is how the bug was discovered in the first place - by investigating a regression in a downstream project caused by updating Twisted.

In this specific case, I think the least impactful thing is to fix the code, rather than the docs.

The code fix is needed in either case, because if we fix the docs, we need to adjust the behavior of how spawnProcess works in the case when regular fork is used.

[p12tic]

My understanding is that execvpe always requires an explicit environment, and it will not copy the environment from the parent process.

execvpe uses os.environ when env argument is None. See here: https://github.com/python/cpython/blob/3.12/Lib/os.py#L594

[adiroiban]

Many thanks, Povilas for the PR and the explanations and investigation.

With the latest info, I also think that is best to fix the code.

I left a few minor comments regarding the scope/purpose of each test.

I tried to read the code, but I can't find why fork is expected to be used in those tests.

Please check my commend and add your feedback.

Other than that, this can be merged.

Once merged, I will try to trigger a new release.

Thanks again.

[p12tic]

please review

[adiroiban]

Many thanks for the PR.

It looks good.

My only blocker for this PR is trying to use a dedicated reactor instance, rather than using the global reactor.

Maybe it's a bad suggestion :)

See my inline comments. Thanks!

[p12tic]

please review

[p12tic]

@adiroiban

Thanks a lot for your suggestions. I think I addressed all comments.

[glyph]

Looks great. Thanks for integrating all that feedback.

review comments addressed