Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-132413: Fix crash in _datetime when used at shutdown (alt)#132665
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
neonene commented Apr 18, 2025 • 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.
python-cla-botbot commented Apr 18, 2025 • 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.
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.
ZeroIntensity 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.
I don't think we should be storing more things in the interpreter dictionary. It's relatively slow, and at a glance, probably a thread-safety issue in the way it's being used here.
The better solution would be to drop the interpreter dictionary usage here entirely, and just store the module state on the interpreter state.
neonene commented Apr 20, 2025
It sounds like the concern is not about the issue that needs to be fixed even on 3.13. The current module state access may not be so fast, but I doubt this PR makes it slower. My question for alternatives is that how much we need to care about the thread-safety after |
ZeroIntensity commented Apr 20, 2025
I meant an existing thread-safety issue, but you're right, let's not address that here.
|
neonene commented Apr 20, 2025 • 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 agree to the risk of manual invocation during finalization. Two versions work fine:
Both require the interpreter dict. I failed to add a module object to the interpreter state (bad reloading). |
ZeroIntensity commented Apr 20, 2025
The interpreter state isn't part of the public ABI, we can (and do) change it in backports. |
Uh oh!
There was an error while loading. Please reload this page.
ZeroIntensity commented May 1, 2025
Let me know when you're ready for a re-review. |
neonene commented May 1, 2025 • 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.
Ready to be reviewed. Though, there seems to be fundamental fixes for 3.14? |
neonene commented May 4, 2025
FTR, I'm aware of: which is still opaque to me, though. |
neonene commented May 8, 2025
Updated another PR (#132599), where |
Create a module when it is not importable.This patch gives the interpreter state struct an additional pointer to the
_datetimemodule state. Currently, the module state can be reached via a weak reference to the module, which seems problematic because the weakref counts the active module as invalid earlier at shutdown.(No different than before logically, but the attempt would need to be withdrawn if there should be a reasonable case that the introduced pointer becomes a dangling pointer or one to a reallocated region. #132599 is an alternative.)
(This does not suppress
ImportErrorraised when running thestrptimemethods at shutdown, which is almost traditional.)datetime.timedelta(possibly from datetime's Cdelta_new) #132413