Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
bpo-42972: Fully support GC for _winapi.Overlapped#26381
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Fidget-Spinner commented May 26, 2021 • edited by bedevere-bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-bot
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner commented May 26, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Anyone knows a surefire way to test this? My main dev machine is Windows, but it seems that >>>importsys,gc>>>for_inrange(5): ... print(sys.gettotalrefcount()) ... import_winapi ... delsys.modules['_winapi'] ... del_winapi ... gc.collect() ... 504680510760514320517880521440Not sure how to test just the |
Fidget-Spinner commented May 26, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 |
erlend-aasland commented May 26, 2021
Only if there's nothing to clear/traverse? :) |
Fidget-Spinner commented May 26, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
there are over 100 objects created on module exec :(. Edit: also no m_free |
Uh oh!
There was an error while loading. Please reload this page.
erlend-aasland left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…for real this time ;-)
Fidget-Spinner commented May 26, 2021
Ok seems things are better now after introducing the module cleanup functions: >>>importsys,gc>>>for_inrange(5): ... print(sys.gettotalrefcount()) ... import_winapi ... delsys.modules['_winapi'] ... del_winapi ... gc.collect() ... 50524425059042505904250590425059042 |
erlend-aasland commented May 26, 2021
That looks nice! If this is a reliable test method, we should use it, at least for the multi-init modules. |
Fidget-Spinner commented May 26, 2021
Thanks. I shamelessly copied it from your message on the bpo. So thanks for the good test and the reviews :). |
erlend-aasland commented May 26, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I'm pretty sure I shamelessly stole it from @encukou: msg393508, issue 44116 😀 |
erlend-aasland commented May 26, 2021
Regarding the tests. I propose to add a |
Uh oh!
There was an error while loading. Please reload this page.
| CloseHandle(self->overlapped.hEvent); | ||
| SetLastError(err); | ||
| PyObject_GC_UnTrack(self); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to start by untracking the GC, at the start to the dealloc function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
miss-islington commented May 28, 2021
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 <28750310+Fidget-Spinner@users.noreply.github.com>
bedevere-bot commented May 28, 2021
GH-26426 is a backport of this pull request to the 3.10 branch. |
vstinner commented May 28, 2021
I'm not fully satisfied by the dealloc function which calls untrack late, but this code is already ugly and the evil |
Fidget-Spinner commented May 28, 2021
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! |
Fidget-Spinner commented May 28, 2021
@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 ;-). |
miss-islington commented May 28, 2021
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 <28750310+Fidget-Spinner@users.noreply.github.com>
bedevere-bot commented May 28, 2021
GH-26427 is a backport of this pull request to the 3.10 branch. |
erlend-aasland commented May 28, 2021
I agree with everything you said there, @Fidget-Spinner :) |
miss-islington commented May 28, 2021
Thanks @Fidget-Spinner for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
bedevere-bot commented May 28, 2021
GH-26431 is a backport of this pull request to the 3.10 branch. |
(cherry picked from commit 0fa282c) Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>
bedevere-bot commented May 28, 2021 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
|
bedevere-bot commented May 28, 2021
|
https://bugs.python.org/issue42972