[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Steve Dower]

Steve Dower  added the comment:

> import subprocess
> subprocess._cleanup = lambda: None

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Sylvain Corlay]

Sylvain Corlay  added the comment:

Yes, I understand that.

I was wondering if there was a workaround that we could use to not drop 3.6
support right away!

On Fri, May 29, 2020, 19:29 STINNER Victor  wrote:

>
> STINNER Victor  added the comment:
>
> Sadly, 3.6 no longer get bug fixes, only security fixes:
> https://devguide.python.org/#status-of-python-branches
>
> --
>
> ___
> Python tracker 
> 
> ___
>

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

Sadly, 3.6 no longer get bug fixes, only security fixes:
https://devguide.python.org/#status-of-python-branches

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Sylvain Corlay]

Sylvain Corlay  added the comment:

Hello,

Is there a means to work around that bug for Python 3.6 ? It seems that the fix 
was only backported to 3.7, and we were not planning on dropping Python 3.6 
support quite yet in our project.

--
nosy: +sylvain.corlay

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Eryk Sun]

Eryk Sun  added the comment:

> When a Popen instance is finalized by the garbage collector, the 
> internal handle is also finalized and closed despite the instance 
> being put on the active list. This results in _cleanup throwing 
> because the handle can no longer be used.

Thanks. I hadn't considered that. It can lead to unusual behavior like this 
when __del__ keeps an object alive. The object can't assume that referenced 
objects haven't already been finalized. All of the subsequently required 
instance methods should take this into account. For example, 
Popen._internal_poll could set the return code to -1 if self._handle.closed is 
true. That's at a higher level, and much more reasonable, than trying to handle 
ERROR_INVALID_HANDLE, which could mask bugs.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

Steve Dower: "I triggered automatic backports to see how they go. Doesn't 
necessarily mean we'll merge them without looking more closely, but may as well 
check out what our automated tools suggest."

Thanks, I merged the backports.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

Thanks Ruslan Kuprieiev for the bug report and the fix. Thanks Eryk Sun (as 
usual!) for the great analysis of this issue.

Ok, the bug should now be fixed in 3.7, 3.8 and master branches.

I chose to not fix Python 2.7: see my previous comment.

See bpo-37410 for the follow-up: "[Windows] subprocess: close the handle when 
the process completes".

--
resolution:  -> fixed
stage: backport needed -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 1e2707d7e82aedf73c59772bc7aa228286323c3c by Victor Stinner (Miss 
Islington (bot)) in branch '3.7':
bpo-37380: subprocess: don't use _active on win (GH-14360) (GH-15706)
https://github.com/python/cpython/commit/1e2707d7e82aedf73c59772bc7aa228286323c3c


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 4d1abedce9422473af2ac78047e55cde73208208 by Victor Stinner (Miss 
Islington (bot)) in branch '3.8':
bpo-37380: subprocess: don't use _active on win (GH-14360) (GH-15707)
https://github.com/python/cpython/commit/4d1abedce9422473af2ac78047e55cde73208208


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

> When a Popen instance is finalized by the garbage collector, the internal 
> handle is also finalized and closed despite the instance being put on the 
> active list. This results in _cleanup throwing because the handle can no 
> longer be used.

Right, that's a more practical case where this bug still occurs on Python 3.8 
and older. I would suggest to fix applications which should get a 
ResourceWarning warning in this case.

Chip Lynch wrote me an email saying that his team is impacted by the bug on 
Windows with Python 3.7.

I tried to write a simpler patch ignoring ERROR_INVALID_HANDLE, but then I read 
again this issue discussion: Eryk Sun and me agree that ignorning 
ERROR_INVALID_HANDLE is a bad idea, it can be worse: silence a real bug.

So I now agree to backport the fix from master to 3.7 and 3.8 branches.

I prefer to leave 2.7 unchanged even if it's affected. I don't want to take the 
risk of introducing another regression in 2.7. Moreover, subprocess.Popen has a 
__del__() method. Python 2.7 handles reference cycles differently than Python 
3: it doesn't implement finalizers (PEP 442). While Python 2.7 might be 
affected, it should be affected differently.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Steve Dower]

Steve Dower  added the comment:

I triggered automatic backports to see how they go. Doesn't necessarily mean 
we'll merge them without looking more closely, but may as well check out what 
our automated tools suggest.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[miss-islington]

Change by miss-islington :


--
pull_requests: +15361
pull_request: https://github.com/python/cpython/pull/15707

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[miss-islington]

Change by miss-islington :


--
pull_requests: +15360
pull_request: https://github.com/python/cpython/pull/15706

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Steve Dower]

Change by Steve Dower :


--
stage: patch review -> backport needed
versions: +Python 3.8, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Chip Lynch]

Chip Lynch  added the comment:

I see there is a merged patch for this 
(https://github.com/python/cpython/pull/14360); is it possible to get it tagged 
for backport to 3.7?  I haven't seen the 3.7.5 tag drop yet so wanted to 
recommend it's included, it's giving us fits.  On our Windows 10 dev boxes and 
Win 2016 servers, emanuel's repro fails 3.5.4, 3.6.8, 3.7.4, and 3.8.0b4, but 
not apparently 2.7 (I think we tested 2.7.12).  Per vstinner's request, I agree 
and would like to see it get a 3.7 and 3.8 backport.

Many thanks!

--
nosy: +Chip Lynch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Emanuel Zephir]

Emanuel Zephir  added the comment:

I believe this to be a bug in the standard library instead of solely being the 
result of an application error.

When a Popen instance is finalized by the garbage collector, the internal 
handle is also finalized and closed despite the instance being put on the 
active list. This results in _cleanup throwing because the handle can no longer 
be used.

I've attached a small reproduction.

--
nosy: +emanuel
Added file: https://bugs.python.org/file48493/invalid_handle.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

> Say a library calls CreateEventW and gets handle 32. It passes this handle to 
> some other library, which uses the event and closes the handle when it no 
> longer needs it. But due to a miscommunication in the documentation, the 
> first library thinks the handle remains open. Now handle 32 is free for 
> reuse, but the library doesn't know this. subprocess.Popen subsequently calls 
> CreateProcessW and gets handle 32. Later on, the library closes handle 32, 
> making it invalid, at least until it gets assigned to some other kernel 
> object.

So yeah, it's a severe bug in an application. An application should not close 
the wrong handle by mistake :-) 

Thanks for the explanation. So it would be "nice" to backport the change to 
reduce the impact of such application bug, but it's not really a bug in Python 
itself. Python cannot fully protect developers for such class of bugs.

On Unix, there is a similar bug with applications trying to close a file 
descriptor which is already closed. It can lead to very severe bugs (crashes):
https://bugs.python.org/issue18748

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Eryk Sun]

Eryk Sun  added the comment:

> Without accessing private attributes, I don't see how someone can 
> discover the private handle. So for me, it's more a serious bug in an 
> application, no? Blindly closing random handles doesn't sound like a 
> good idea to me.

Say a library calls CreateEventW and gets handle 32. It passes this handle to 
some other library, which uses the event and closes the handle when it no 
longer needs it. But due to a miscommunication in the documentation, the first 
library thinks the handle remains open. Now handle 32 is free for reuse, but 
the library doesn't know this. subprocess.Popen subsequently calls 
CreateProcessW and gets handle 32. Later on, the library closes handle 32, 
making it invalid, at least until it gets assigned to some other kernel object.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

> The example is just emulating a problem from someone else's code that's 
> closing our handle. Typically this situation occurs because the code is 
> holding onto a handle value for a kernel object (File, Section, Job, Process, 
> Thread, Event, etc) that got closed.

Without accessing private attributes, I don't see how someone can discover the 
private handle. So for me, it's more a serious bug in an application, no? 
Blindly closing random handles doesn't sound like a good idea to me.

> The handle value eventually gets reused, such as for our _handle.

That's a side effect on the blindly closing random handles.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Eryk Sun]

Eryk Sun  added the comment:

>> subprocess._active[0]._handle.Close()
>
> Why would you do that? You should not access the private _active list, 
> nor access the private _handle attribute. I understand that it's a way 
> to trigger such bug, but is it possible to trigger this bug without 
> accessing any private attribute?

The example is just emulating a problem from someone else's code that's closing 
our handle. Typically this situation occurs because the code is holding onto a 
handle value for a kernel object (File, Section, Job, Process, Thread, Event, 
etc) that got closed. The handle value eventually gets reused, such as for our 
_handle. 

>> I wouldn't want _internal_poll to silence this error, but maybe it 
>> could be translated into a warning
>
> I disagree with that. It's very bad is suddenly the handle becomes 
> invalid for no obvious reason. It's better to get an hard error 
> (exception) in such case.

I agree, but the exception breaks Popen.__init__. It would have to be ignored 
or translated to a warning somewhere if we continues to poll a list of active 
processes every time __init__() is called. But since the latter is unnecessary 
in Windows, I suggested to just skip it.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

> subprocess._active[0]._handle.Close()

Why would you do that? You should not access the private _active list, nor 
access the private _handle attribute. I understand that it's a way to trigger 
such bug, but is it possible to trigger this bug without accessing any private 
attribute?


> I wouldn't want _internal_poll to silence this error, but maybe it could be 
> translated into a warning

I disagree with that. It's very bad is suddenly the handle becomes invalid for 
no obvious reason. It's better to get an hard error (exception) in such case.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Eryk Sun]

Eryk Sun  added the comment:

>> If one of the processes in that list is gone, then (...) "The handle 
>> is invalid" (...) will prevent you from creating any new processes 
>> with Popen after that
>
> It's the first time that I see such bug report.

IIRC it's the second time I've seen that issue reported. It should be rare. It 
should only occur if some other code in our process or another process closes 
our _handle value. It's not our bug. Here's an example in 3.7:

>>> p = subprocess.Popen('python -c "input()"', stdin=subprocess.PIPE)
>>> del p
>>> subprocess._active
[]
>>> subprocess._active[0]._handle.Close()

The process is active, but something closed our handle. The next time 
Popen.__init__ calls _cleanup, _internal_poll raises OSError due to the invalid 
handle:

>>> subprocess.call('python -V')
Traceback (most recent call last):
  File "", line 1, in 
  File "C:\Program Files\Python37\lib\subprocess.py", line 323, in call
with Popen(*popenargs, **kwargs) as p:
  File "C:\Program Files\Python37\lib\subprocess.py", line 664, in __init__
_cleanup()
  File "C:\Program Files\Python37\lib\subprocess.py", line 228, in _cleanup
res = inst._internal_poll(_deadstate=sys.maxsize)
  File "C:\Program Files\Python37\lib\subprocess.py", line 1216, in 
_internal_poll
if _WaitForSingleObject(self._handle, 0) == _WAIT_OBJECT_0:
OSError: [WinError 6] The handle is invalid

I wouldn't want _internal_poll to silence this error, but maybe it could be 
translated into a warning. That said, since there's no need to wait on a 
process in Windows, there's no need for __del__ to insert a running process in 
a list of active process, in which case there's nothing for _cleanup to do. 
Given we're in __del__, our private _handle is about to be closed, at least it 
will be in CPython. Other implementations should wait() or use a with 
statement, in order to explicitly close the process handle. The latter isn't 
implemented currently, but you opened issue 37410 to address this.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

> To me, it doesn't seem dangerous to backport that, but I would love to hear 
> what Eryk and others think :)

In my experience, any change is dangerous :-)

The initial bug doesn't contain any reproducer. I'm talking about:

> If one of the processes in that list is gone, then (...) "The handle is 
> invalid" (...) will prevent you from creating any new processes with Popen 
> after that

It's the first time that I see such bug report. It seems like subprocess works 
for most people :-) Can you elobrate how to reproduce this issue to justify a 
backport?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Ruslan Kuprieiev]

Ruslan Kuprieiev  added the comment:

The `_active` code is pretty old and has not changed since 2.7, so it makes 
sense to backport it for now(also, should be very trivial to do that). To me, 
it doesn't seem dangerous to backport that, but I would love to hear what Eryk 
and others think :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

I merged PR 14360 into master. Are Python 2.7, 3.7 and 3.8 impacted by the bug 
as well? Should we backport the change, or would it be too dangerous?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:


New changeset 042821ae3cf537e01963c9ec85d1a454d921e826 by Victor Stinner 
(Ruslan Kuprieiev) in branch 'master':
bpo-37380: subprocess: don't use _active on win (GH-14360)
https://github.com/python/cpython/commit/042821ae3cf537e01963c9ec85d1a454d921e826


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

Here is a concrete example of ResourceWarning when debugging/stressing 
multiprocessing code, I interrupt (CTRL+c) test_resource_tracker while it's 
running:

vstinner@apu$ ./python -m test test_multiprocessing_spawn --fail-env-changed -v 
-m test_resource_tracker
== CPython 3.9.0a0 (heads/master:689830ee62, Jun 26 2019, 23:07:31) [GCC 9.1.1 
20190503 (Red Hat 9.1.1-1)]
== Linux-5.1.11-300.fc30.x86_64-x86_64-with-glibc2.29 little-endian
== cwd: /home/vstinner/prog/python/master/build/test_python_18573
== CPU count: 8
== encodings: locale=UTF-8, FS=utf-8
Run tests sequentially
0:00:00 load avg: 0.32 [1/1] test_multiprocessing_spawn
test_resource_tracker (test.test_multiprocessing_spawn.TestResourceTracker) ... 

^C

/home/vstinner/prog/python/master/Lib/subprocess.py:917: ResourceWarning: 
subprocess 18576 is still running
  _warn("subprocess %s is still running" % self.pid,
ResourceWarning: Enable tracemalloc to get the object allocation traceback
/home/vstinner/prog/python/master/Lib/test/libregrtest/runtest.py:283: 
ResourceWarning: unclosed file <_io.BufferedReader name=6>
  return INTERRUPTED
ResourceWarning: Enable tracemalloc to get the object allocation traceback

== Tests result: INTERRUPTED ==
Test suite interrupted by signal SIGINT.

1 test omitted:
test_multiprocessing_spawn

Total duration: 304 ms
Tests result: INTERRUPTED

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

Sorry, I'm lost :-( More and more different issues are discussed here. Would it 
be possible please to stick this issue to _active list and _cleanup() on 
Windows? From what I read, remove/disable _active and _cleanup() makes sense on 
Windows.

I already created bpo-37410 to call CloseHandle() earlier.

Eryk: Please open a separated issue if you want to change the ResourceWarning 
message.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Eryk Sun]

Eryk Sun  added the comment:

> The process is not polled to be able to emit the warning in more 
> cases, again, to ease debug.

It emits an incorrect warning if the process has already exited: "subprocess %s 
is still running". This can be rectified. Here's my current understanding:

def __del__(self, _maxsize=sys.maxsize, _warn=warnings.warn):
if not self._child_created or self.returncode is not None:
return
# In Unix, not reading the subprocess exit status creates a zombie
# process, which is only destroyed at the parent Python process exit.
# In Windows, no one else should have a reference to our _handle, so
# it should get finalized and thus closed, but we use the same warning
# in order to consistently educate developers.
if self._internal_poll(_deadstate=_maxsize) is not None:
_warn("subprocess %s was implicitly finalized" % self.pid,
  ResourceWarning, source=self)
else:
_warn("subprocess %s is still running" % self.pid,
  ResourceWarning, source=self)
# Keep this instance alive until we can wait on it, if needed.
if _active is not None:
_active.append(self)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

> In Unix, the zombie remains visible in the task list (marked as  in 
> Linux), but in Windows an exited process is removed from the Process 
> Manager's active list, so it's no longer visible to users. Also, a Process 
> object is reaped as soon as the last reference to it is closed, since clearly 
> no one needs it anymore. 
> (...)
> We could call self._handle.Close() in _wait(), right after calling 
> GetExitCodeProcess(self._handle). With this change, __exit__ will ensure that 
> _handle gets closed in a deterministic context.

I created bpo-37410 and PR 14391 to close the handle when the process 
completes. IMHO this change is easy and safe, it can be backported to Python 
3.7 and 3.8.

> Code that needs the handle indefinitely can call _handle.Detach() before 
> exiting the with-statement context, but that should rarely be necessary.
> (...)
> There should be a way to indicate a Popen instance is intended to continue 
> running detached from our process, so scripts don't have to ignore an 
> irrelevant resource warning.

My point is that if you want to "detach" a process, it must be *explicit*. In 
my experience, processes are "leaked" implicitly: by mistake. In 
multiprocessing, it can be subtle, you don't manipulate subprocess.Popen 
directly.

Users must not access private attributes (Popen._handle). After I added the 
ResourceWarning, I created bpo-27068 "Add a detach() method to 
subprocess.Popen". But I only created to reply to a request of Martin Panter, I 
didn't use this feature myself. At some point, I decided to just close the 
issue. Maybe it's time to reopen it.


> I don't understand emitting a resource warning in Popen.__del__ if a process 
> hasn't been waited on until completion beforehand (i.e. self.returncode is 
> None). If a script wants to be strict about this, it can use a with 
> statement, which is documented to wait on the process.

The purpose is to help developers to find bugs in their bugs. Many are now 
aware that programs continue to run in the background and that leaking zombie 
processes is an issue on Unix.

The process is not polled to be able to emit the warning in more cases, again, 
to ease debug.

The warning is ignored by default.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Eryk Sun]

Eryk Sun  added the comment:

> One issue on Linux is that the zombie process keeps the pid used until 
> the parent reads the child exit status, and Linux pids are limited to
> 32768 by default.

Windows allocates Process and Thread IDs out of a kernel handle table, which 
can grow to about 2**24 entries (more than 16 million). So the practical 
resource limit for inactive Process and Thread objects is available memory, not 
running out of PID/TID values.

> Linux (for example) has the same design: the kernel doesn't keep a 
> "full process" alive, but a lightweight structure just for its parent
> process which gets the exit status. That's the concept of "zombie 
> process".

In Unix, the zombie remains visible in the task list (marked as  in 
Linux), but in Windows an exited process is removed from the Process Manager's 
active list, so it's no longer visible to users. Also, a Process object is 
reaped as soon as the last reference to it is closed, since clearly no one 
needs it anymore. 

> The subprocess module uses a magic Handle object which calls 
> CloseHandle(handle) in its __del__() method. I dislike relying on 
> destructors. If an object is kept alive by a reference cycle, it's
> never released: CloseHandle() isn't called.

We could call self._handle.Close() in _wait(), right after calling 
GetExitCodeProcess(self._handle). With this change, __exit__ will ensure that 
_handle gets closed in a deterministic context. Code that needs the handle 
indefinitely can call _handle.Detach() before exiting the with-statement 
context, but that should rarely be necessary.

I don't understand emitting a resource warning in Popen.__del__ if a process 
hasn't been waited on until completion beforehand (i.e. self.returncode is 
None). If a script wants to be strict about this, it can use a with statement, 
which is documented to wait on the process. 

I do understand emitting a resource warning in Popen.__del__ if 
self._internal_poll() is None. In this case, in Unix only now, the process gets 
added to the _active list to try to avoid leaking a zombie. The list gets 
polled in subprocess._cleanup, which is called in Popen.__init__. Shouldn't 
_cleanup also be set as an atexit function?

There should be a way to indicate a Popen instance is intended to continue 
running detached from our process, so scripts don't have to ignore an 
irrelevant resource warning.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Steve Dower]

Steve Dower  added the comment:

The handle can deliberately live beyond the process, but it does not have to. 
If it is closed early then the kernel object will be freed when no handles 
remain, which will be at process exit.

So it's a classic __exit__/__del__ case, where both are needed if you want 
deterministic resource disposal. But it is in no way tied to the life of the 
child process, so waiting first is optional.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

> See issue 36067 for a related discussion. The _active list and _cleanup 
> function are not required in Windows. When a process exits, it gets rundown 
> to free its handle table and virtual memory. Only the kernel object remains, 
> which is kept alive by pointer and handle references to it that can query 
> information such as the exit status code. As soon as the last reference is 
> closed, the Process object is automatically reaped. It doesn't have to be 
> waited on.

Linux (for example) has the same design: the kernel doesn't keep a "full 
process" alive, but a lightweight structure just for its parent process which 
gets the exit status. That's the concept of "zombie process".

One issue on Linux is that the zombie process keeps the pid used until the 
parent reads the child exit status, and Linux pids are limited to 32768 by 
default.

Now I'm not sure that I understand what it means on Windows. The subprocess 
module uses a magic Handle object which calls CloseHandle(handle) in its 
__del__() method. I dislike relying on destructors. If an object is kept alive 
by a reference cycle, it's never released: CloseHandle() isn't called.

So code spawning process should wait until subprocesses complete to ensure that 
CloseHandle() is called, no?

Except that Popen._internal_poll() doesn't clear the handle even after the 
process completes.

If Popen.__del__() doesn't add active processes to the _active list, it should 
at least explicitly call CloseHandle(), no?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Ruslan Kuprieiev]

Ruslan Kuprieiev  added the comment:

Thanks for pointing that out! I've submitted a tiny patch to skip 
``_collect()`` and ``__del__``. Please find it in the Github PR attached. 
Looking forward to your feedback!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Ruslan Kuprieiev]

Change by Ruslan Kuprieiev :


--
pull_requests: +14176
pull_request: https://github.com/python/cpython/pull/14360

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[STINNER Victor]

STINNER Victor  added the comment:

> 2) ignore EBADF(OSError: [WinError 6] The handle is invalid) in terminate() 
> and probably some other methods

That sounds like a bad idea. We should avoid using a handle if it can become 
invalid out of our control. A handle might be recycled, no?

--
nosy: +vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Eryk Sun]

Eryk Sun  added the comment:

> 1) disable _cleanup, _active, and remove _internal_poll() for windows

_active only gets appended to in __del__. We can skip the entire body of 
__del__. Also, calling _cleanup can be skipped in __init__.

_internal_poll is required for poll().

2) ignore EBADF(OSError: [WinError 6] The handle is invalid) in terminate() and 
probably some other methods

ERROR_INVALID_HANDLE should not be ignored. 

terminate() is doing the right thing by not masking ERROR_INVALID_HANDLE. The 
only concern is the case where our handle has been closed elsewhere (not by 
subprocess) and the handle value was subsequently reused as a handle for 
another process to which we have terminate access. This is a bad handle at a 
logical, application level, but it's valid and accessible for TerminateProcess. 
It was suggested to initially get the pid and process startup time in order to 
validate the call, but Alexey Izbyshev is right that it's not worth 
complicating the code in subprocess to address an uncommon bug in particular 
application or library code.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Ruslan Kuprieiev]

Ruslan Kuprieiev  added the comment:

Hi Eryk!

Thanks for a swift reply! So the current plan for fixing it is:

1) disable _cleanup, _active, and remove _internal_poll() for windows
2) ignore EBADF(OSError: [WinError 6] The handle is invalid) in terminate() and 
probably some other methods

Please let me know if I'm missing something. Also, is anyone working on this, 
or should I prepare a PR?

Thanks,
Ruslan

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Roundup Robot]

Change by Roundup Robot :


--
keywords: +patch
pull_requests: +14147
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/14327

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Eryk Sun]

Eryk Sun  added the comment:

See issue 36067 for a related discussion. The _active list and _cleanup 
function are not required in Windows. When a process exits, it gets rundown to 
free its handle table and virtual memory. Only the kernel object remains, which 
is kept alive by pointer and handle references to it that can query information 
such as the exit status code. As soon as the last reference is closed, the 
Process object is automatically reaped. It doesn't have to be waited on.

--
nosy: +eryksun

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
nosy: +gregory.p.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue37380] subprocess.Popen._cleanup() "The handle is invalid" error when some old process is gone

[Ruslan Kuprieiev]

New submission from Ruslan Kuprieiev :

subprocess keeps the list of active processes in its `_active` list. Whenever 
you try to Popen a new process, it is immediately (like right in the first line 
of Popen.__init__ [1]) trying to go clean up the old ones. If one of the 
processes in that list is gone, then

```
(_WaitForSingleObject(self._handle, 0)
```

[2] will get `OSError: [WinError 6] The handle is invalid` and will prevent you 
from creating any new processes with Popen after that, because the line where 
that handle is removes from _active list is not reached. On *nix [3] 
_internal_poll will actually try-except waitpid and will catch OSError without 
re-raising it, so if the process is gone, it will simply report it as dead and 
successfully remove it from _active list. It seems like we should do the same 
thing on Windows and if we catch `The handle is invalid` error, just report 
that process as dead and remove it from _active list.


[1] https://github.com/python/cpython/blob/v3.8.0b1/Lib/subprocess.py#L715

[2] https://github.com/python/cpython/blob/v3.8.0b1/Lib/subprocess.py#L1310

[3] https://github.com/python/cpython/blob/v3.8.0b1/Lib/subprocess.py#L1710

--
components: Windows
messages: 346332
nosy: Ruslan Kuprieiev, paul.moore, steve.dower, tim.golden, zach.ware
priority: normal
severity: normal
status: open
title: subprocess.Popen._cleanup() "The handle is invalid" error when some old 
process is gone
type: crash
versions: Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com