bpo-42972: Fully support GC for _winapi.Overlapped #26381
Conversation
|
Anyone knows a surefire way to test this? My main dev machine is Windows, but it seems that >>> import sys,gc
>>> for _ in range(5):
... print(sys.gettotalrefcount())
... import _winapi
... del sys.modules['_winapi']
... del _winapi
... gc.collect()
...
50468
0
51076
0
51432
0
51788
0
52144
0Not sure how to test just the |
|
Wait a minute, are multi phase init modules supposed to have a missing m_clear and m_traverse? https://github.com/python/cpython/blob/main/Modules/_winapi.c#L2046-L2052 |
Only if there's nothing to clear/traverse? :) |
there are over 100 objects created on module exec :(. Edit: also no m_free |
…for real this time ;-)
|
Ok seems things are better now after introducing the module cleanup functions: >>> import sys,gc
>>> for _ in range(5):
... print(sys.gettotalrefcount())
... import _winapi
... del sys.modules['_winapi']
... del _winapi
... gc.collect()
...
50524
42
50590
42
50590
42
50590
42
50590
42 |
|
That looks nice! If this is a reliable test method, we should use it, at least for the multi-init modules. |
Thanks. I shamelessly copied it from your message on the bpo. So thanks for the good test and the reviews :). |
I'm pretty sure I shamelessly stole it from @encukou: msg393508, issue 44116 😀 |
|
Regarding the tests. I propose to add a |
| @@ -150,6 +159,7 @@ overlapped_dealloc(OverlappedObject *self) | |||
|
|
|||
| CloseHandle(self->overlapped.hEvent); | |||
| SetLastError(err); | |||
| PyObject_GC_UnTrack(self); | |||
There was a problem hiding this comment.
I would prefer to start by untracking the GC, at the start to the dealloc function.
There was a problem hiding this comment.
I initially did that, but a concern was brought up here which I think makes sense #26381 (comment).
There was a problem hiding this comment.
If tp_dealloc returns without clearing the object, IMO it's better to remove it from the GC. It's better to hide this badly broken Python object. I suggest to start by untracking and explain that it's a deliberate choice to untrack even if we hit the worst case scenario of "return" below.
Note: IMO this code should be removed, Windows XP is no longer supported, but it should be done in a separated change.
There was a problem hiding this comment.
Sorry, I just introduced a regression with this, seems like there's a race condition with the GC and the IO operation somewhere.
> python_d.exe -m test test_httplib -m test_local_bad_hostname
...\cpython\Modules\gcmodule.c:446: update_refs: Assertion "gc_get_refs(gc) != 0" failed
Enable tracemalloc to get the memory block allocation traceback
object address : 000002DADE2A99D0
object refcount : 0
object type : 000002DADDF72F60
object type name: _winapi.Overlapped
object repr : <refcnt 0 at 000002DADE2A99D0>
Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed
Python runtime state: initialized
It seems that placing the PyObject_GC_UnTrack at the top actually fixes it...
|
Thanks @Fidget-Spinner for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
(cherry picked from commit 0fa282c) Co-authored-by: Ken Jin <[email protected]>
|
GH-26426 is a backport of this pull request to the 3.10 branch. |
|
I'm not fully satisfied by the dealloc function which calls untrack late, but this code is already ugly and the evil |
Sorry i was going to move the untrack up, but timezones and work got in the way.
I hope so, Win7 is still pretty popular. WinXP is almost gone except for some niche areas if this site is to be believed. https://gs.statcounter.com/os-version-market-share/windows/desktop/worldwide Anyways, I appreciate the reviews and merge. Thanks! |
@erlend-aasland I think this is a worthwhile test to add in the future to make sure we don't regress (especially for module finalization reference leaks, I'm not too worried about the individual types with/without GC). Though I'm waiting for the dust to settle and see what the people on python-committers have to say about this issue specifically. (I'm also using it as an excuse to let everyone take a break. I think Victor has been working a little too hard reviewing all our PRs ;-). |
|
Thanks @Fidget-Spinner for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
(cherry picked from commit 0fa282c) Co-authored-by: Ken Jin <[email protected]>
|
GH-26427 is a backport of this pull request to the 3.10 branch. |
|
I agree with everything you said there, @Fidget-Spinner :) |
|
Thanks @Fidget-Spinner for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
|
GH-26431 is a backport of this pull request to the 3.10 branch. |
(cherry picked from commit 0fa282c) Co-authored-by: Ken Jin <[email protected]>
|
|
https://bugs.python.org/issue42972