[Python-Dev] r87010 - in python/branches/py3k: Doc/library/subprocess.rst Lib/subprocess.py Lib/test/test_subprocess.py

[Paul Moore]

On 4 December 2010 20:13, Gregory P. Smith <greg at krypto.org> wrote:
>> This sounds like omitting the close_fds parameter is now considered
>> ill-advised, if not outright wrong.
[...]
> Making the change was intended to force the discussion.  I'm glad that
> worked. :)

:-)

> I don't like the thought of requiring people to specify close_fds either but
> the default is a bad one (see http://bugs.python.org/issue7213
> and http://bugs.python.org/issue2320) so we should change it.

Both of these issues seem to be for Unix (based on the fact that they
use grep and cat as executables - I haven't dug very deeply into
this). I work on Windows only, so I'm happy to take the experts'
opinions on that OS.

Is there an issue on Windows? If not, and given how different FD
inheritance is on Windows, I'd argue that in the absence of bug
reports, there's no need to change behaviour on Windows.

> The real question is how should we go about doing the change?  This warning
> asking people to always specify close_fds=True is heavy handed.  Other
> places in the stdlib and docs no doubt still need to be updated to reflect
> it, etc.
>
> Options that seem worthy of debate:
> A) The DeprecationWarning that exists in py3k as of today.

Given that the option appears not to be getting deprecated (just the
default changed), this seems wrong.

I know DeprecationWarnings are off by default. If someone switches
them on, they'd expect to have to address them. But what should they
do in this case? Unless they have thought hard about FD inheritance,
and then explicitly decided to use the default because it matches what
they want (as opposed to specifying it explicitly, and documenting
their intent clearly), nothing. Or they use the default expecting it
to be good enough, and suddenly have to worry if they are relying on
something that's changing (I have no feel for how likely it is that
they'll be affected by the change, other than to say that personally,
none of my code would be). That seems to me like a bad use of a
DeprecationWarning, as a non-trivial amount of the time there's
nothing to do.

> B) Remove the DeprecationWarning, simply document that people should be
> explicit about it if they care at all and that the default will change in
> 3.3.

That seems reasonable to me. But I can see that the backward
compatibility rules might apply here. On the other hand, given the bug
reports, surely this is a bug fix?

> C) Never change close_fds behavior.  we're stuck with it for life.

Sounds harsh. But look at it another way - if compatibility rules mean
that we can't change the default, the reported bugs need to be fixed
another way. Or behaviour needs to be documented more explicitly so
that the bugs can be downgraded to "user error" caused by not reading
the (new, improved) documentation.

Compatibility rules are to protect working programs. What proportion
of programs using the default are "working" and what proportion are
"wrong"? It seems to me that most are "working", so compatibility
applies.

> D) Deprecate close_fds so that it becomes a legacy no-op.  the new pass_fds
> flag could evolve into this.  this will break existing code that depends on
> close_fds one way or another.

This works for me, as I never specify close_fds.

> I'm in 100% agreement that forcing people to pass close_fds in makes the API
> less friendly (granted, people already find it unfriendly but why make it
> worse?).

I'd say making the parameter mandatory is highly unfriendly - to the
point of making the API feel like an "advanced" one, not suitable for
simple cases like replacing os.system.

> Option B seems the most friendly to me.
> Option D is always on the table but I haven't planned out what a future
> without it should look like.  I prefer requiring people who need open file
> descriptors to pass them in; a semaphore for "all fds" could be created and
> pass_fds=ALL becomes the new "close_fds=False" (I think one of the bugs
> suggests this).

In my view:

1. Don't change Windows behaviour unless there's a demonstrable issue.
The Popen code is split into 2 distinct paths internally anyway, so
that shouldn't be a problem. If documenting the behaviour becomes
awkward should Unix behaviour change and Windows doesn't, that
indicates to me that there's a problem with the behaviour ("if it's
hard to explain...").

2. Users who don't specify close_fds because they don't care (as
opposed to because they explicitly want the current default, but
choose not to say so - I appreciate that this is a fine distinction,
difficult to police in practice) should see no change in behaviour,
apart from identified bugs being fixed. If you're not hitting a bug,
you should see no change at all. Keep the interface straightforward
for people who don't know (or don't want to worry about) the
subtleties of FD inheritance.

The original goal was for subprocess to replace os.system, os.popen,
os.spawn, etc. That's never quite happened because subprocess is just
a little bit too conceptually complex for those basic tasks. Let's not
make it worse!

Paul.