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

subprocess: restore backwards-compatibility with python2 #1063

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@ralt
Contributor

ralt commented Dec 31, 2017

In python2, restore_signals doesn't exist, and the default behavior is
to not restore the signals. (This can be done with preexec_fn, if
wanted.) By setting the option to True by default, and not allowing it
to be changed, the behavior becomes mandatory, and is a change from
cpython's behavior on python 2.7.

This change sets the correct default option for py2, and leaves the
py3 version still customizable, maintaining compatibility in both
versions.

subprocess: restore backwards-compatibility with python2
In python2, restore_signals doesn't exist, and the default behavior is
to not restore the signals. (This can be done with preexec_fn, if
wanted.) By setting the option to True by default, and not allowing it
to be changed, the behavior becomes mandatory, and is a change from
cpython's behavior on python 2.7.

This change sets the correct default option for py2, and leaves the
py3 version still customizable, maintaining compatibility in both
versions.
@jamadden

This comment has been minimized.

Member

jamadden commented Jan 8, 2018

Thanks for this PR. Fidelity to the standard library is an important concern of gevent.

The zen says "practicality beats purity", and here that can cut both ways.

The argument for the current behaviour is that having the (few) signals that Python ignores by default be restored for exec'd children is generally a more sane behaviour, which is why it was made the default in Python 3. I don't see the Python 2 behaviour documented, nor do I find any Python 2 tests in the standard library broken by changing it, so I would tend to consider it to be "implementation defined" behaviour, and as such I would prefer not to have the sanity removed by default under Python 2 again.

Can you talk a little bit about what problems you experienced as a result of this to provide context for the other side?

I would support allowing restore_signals as an argument under Python 2 if that would solve your concerns. (I'm not entirely sure why the kwargs check is so strict, but perhaps there's a standard library test that checks for that---although we have the non-standard argument threadpool so that seems unlikely.)

@ralt

This comment has been minimized.

Contributor

ralt commented Jan 8, 2018

Hi,

I checked out cpython source to figure out that the default behavior was to not restore the signals.

The bug occured because we run a python process as a systemd service; systemd masks sigpipe for all its services by default. (The reasoning being that except for shells, sigpipe is useless and just a chore. It does provide a way to not mask sigpipe.) Only when running a subprocess were we getting sigpipe instead of EPIPE, which was killing the subprocess, which was very odd because of systemd's behavior that we expected.

We have solved this by fixing the sigpipe to begin with, but gevent's behavior was surprising given that default behavior of py2's stdlib.

So, in and of itself, we don't need this PR to be merged, it just fixes what we thought was an accidental bug.

@jamadden

This comment has been minimized.

Member

jamadden commented Jan 8, 2018

I checked out cpython source to figure out that the default behavior was to not restore the signals.

Sure, but what about PyPy or Jython or IronPython or micropy? These are all implementations of Python, and the CPython implementation behaviour does not define the Python specification. gevent does run on at least one of those other implementations.

The bug occured because we run a python process as a systemd service; systemd masks sigpipe for all its services by default.

Thanks for the explanation! So when you upgrade your process to run under Python 3, you'll have to take the additional step of adding restore_signals=False to your relevant subprocess calls. gevent could potentially ease the transition if it supported that argument...

So, in and of itself, we don't need this PR to be merged, it just fixes what we thought was an accidental bug.

OK, cool. I'll leave this open as a reminder to improve the documentation and/or enable the restore_signals argument.

@jamadden jamadden added this to the 1.3 milestone Jan 26, 2018

jamadden added a commit that referenced this pull request Jan 29, 2018

Allow all keyword arguments to Popen under Python 2.
And make restore_signals default to False on Python 2.

Fixes #1063.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment