◐ Shell
reader mode source ↗
Skip to content

gh-71587: Drop local reference cache to _strptime module in _datetime#120224

Merged
ericsnowcurrently merged 10 commits into
python:mainfrom
neonene:strptime
Jun 12, 2024
Merged

gh-71587: Drop local reference cache to _strptime module in _datetime#120224
ericsnowcurrently merged 10 commits into
python:mainfrom
neonene:strptime

Conversation

@neonene

@neonene neonene commented Jun 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

For 3.13 and newer, this PR fixes the following errors by making each loaded _datetime module use its own reference cache to _strptime module. The cache will be the same as the reference in sys.modules of the corresponding interpreter by importing _strptime module on each strptime() function call.

Main interpreter:

>_testembed_d test_repeated_init_exec "import datetime; datetime.datetime.strptime('200001', '%Y%m')"
--- Loop #1 ---
--- Loop #2 ---
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: 'NoneType' object is not callable

Sub interpreter:

>>> import _interpreters
>>> for i in range(1,3):
...     print('Sub Loop', i)
...     interp = _interpreters.create()
...     s = "import datetime; datetime.datetime.strptime('200001', '%Y%m')"
...     _interpreters.run_string(interp, s)
...     _interpreters.destroy(interp)
...
Sub Loop 1
Sub Loop 2
Assertion failed: PyUnicode_CheckExact(ep_key), file C:\cp\Objects\dictobject.c, line 1126

cc @ericsnowcurrently @erlend-aasland


@neonene neonene requested review from abalkin and pganssle as code owners June 7, 2024 12:21
@bedevere-app bedevere-app Bot added the label Jun 7, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jun 7, 2024
Closed
neonene and others added 2 commits June 7, 2024 22:50
@neonene @erlend-aasland
Apply suggestions from code review …
39a4259 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@neonene
insert a comment
33e0966
@neonene neonene changed the title gh-71587: Fix reference caching to _strptime module in _datetime Jun 7, 2024
kumaraditya303
kumaraditya303 approved these changes Jun 8, 2024
View reviewed changes

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM

@neonene

neonene commented Jun 9, 2024

Copy link
Copy Markdown
Contributor Author

@kumaraditya303 Thanks for the review.

@ericsnowcurrently Maybe off-topic now, but is it possible to access the module state through a static type like:

// pycore_typeobject.h

typedef struct {
    PyTypeObject *type;
    int isbuiltin;
    ....
+   PyObject *current_ext_module  // weak ref? or borrowed ref?
} managed_static_type_state;

Then, get_current_module(&PyDateTime_DateType, &reloading) in _datetimemodule.c?

@neonene neonene mentioned this pull request Jun 9, 2024
Closed
@ericsnowcurrently

ericsnowcurrently commented Jun 12, 2024

Copy link
Copy Markdown
Member

@ericsnowcurrently Maybe off-topic now, but is it possible to access the module state through a static type like:

// pycore_typeobject.h

typedef struct {
    PyTypeObject *type;
    int isbuiltin;
    ....
+   PyObject *current_ext_module  // weak ref? or borrowed ref?
} managed_static_type_state;

Then, get_current_module(&PyDateTime_DateType, &reloading) in _datetimemodule.c?

That definitely could work. It's worth looking into for 3.14.

ericsnowcurrently
ericsnowcurrently approved these changes Jun 12, 2024
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM

@ericsnowcurrently

ericsnowcurrently commented Jun 12, 2024

Copy link
Copy Markdown
Member

This should be backported to 3.13, right?

@neonene

neonene commented Jun 12, 2024

Copy link
Copy Markdown
Contributor Author

I agree to the 3.13 backport.

@ericsnowcurrently ericsnowcurrently merged commit 127c1d2 into python:main Jun 12, 2024
@miss-islington-app

miss-islington-app Bot commented Jun 12, 2024

Copy link
Copy Markdown

Thanks @neonene for the PR, and @ericsnowcurrently for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 12, 2024
@neonene @miss-islington
pythongh-71587: Drop local reference cache to _strptime module in `… …
b091463 
…_datetime` (pythongh-120224)

The _strptime module object was cached in a static local variable (in the datetime.strptime() implementation).  That's a problem when it crosses isolation boundaries, such as reinitializing the runtme or between interpreters.  This change fixes the problem by dropping the static variable, instead always relying on the normal sys.modules cache (via PyImport_Import()).
(cherry picked from commit 127c1d2)

Co-authored-by: neonene <53406459+neonene@users.noreply.github.com>
@bedevere-app

bedevere-app Bot commented Jun 12, 2024

Copy link
Copy Markdown

GH-120424 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jun 12, 2024
@ericsnowcurrently

ericsnowcurrently commented Jun 12, 2024

Copy link
Copy Markdown
Member

I just realized this fixes a long-standing problem and is not related to the recent work on _datetime. I'm guessing it would make sense to backport this to 3.12. (It's too late for 3.11.)

@ericsnowcurrently ericsnowcurrently added the needs backport to 3.12 only security fixes label Jun 12, 2024
@miss-islington-app

miss-islington-app Bot commented Jun 12, 2024

Copy link
Copy Markdown

Thanks @neonene for the PR, and @ericsnowcurrently for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app

miss-islington-app Bot commented Jun 12, 2024

Copy link
Copy Markdown

Sorry, @neonene and @ericsnowcurrently, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 127c1d2771749853e287632c086b6054212bf12a 3.12

@ericsnowcurrently

ericsnowcurrently commented Jun 12, 2024

Copy link
Copy Markdown
Member

@neonene, would you have time to do the 3.12 backport? I'm sure it won't require much effort.

@ericsnowcurrently ericsnowcurrently mentioned this pull request Jun 12, 2024
Closed
ericsnowcurrently pushed a commit that referenced this pull request Jun 12, 2024
@miss-islington @neonene
[3.13] gh-71587: Drop local reference cache to _strptime module in … …
0041087 
…`_datetime` (gh-120424)

The _strptime module object was cached in a static local variable (in the datetime.strptime() implementation).  That's a problem when it crosses isolation boundaries, such as reinitializing the runtme or between interpreters.  This change fixes the problem by dropping the static variable, instead always relying on the normal sys.modules cache (via PyImport_Import()).

(cherry picked from commit 127c1d2, AKA gh-120224)

Co-authored-by: neonene <53406459+neonene@users.noreply.github.com>
@bedevere-app

bedevere-app Bot commented Jun 12, 2024

Copy link
Copy Markdown

GH-120431 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jun 12, 2024
@ericsnowcurrently ericsnowcurrently mentioned this pull request Jun 12, 2024
Merged
@neonene

neonene commented Jun 12, 2024

Copy link
Copy Markdown
Contributor Author

Thanks @ericsnowcurrently, @erlend-aasland for reviewing this, and thanks again @kumaraditya303.

@neonene neonene deleted the strptime branch June 12, 2024 20:32
ericsnowcurrently pushed a commit that referenced this pull request Jun 13, 2024
@neonene
[3.12] gh-71587: Drop local reference cache to _strptime module in … …
7719eef 
…`_datetime` (gh-120431)

The _strptime module object was cached in a static local variable (in the datetime.strptime() implementation).  That's a problem when it crosses isolation boundaries, such as reinitializing the runtme or between interpreters.  This change fixes the problem by dropping the static variable, instead always relying on the normal sys.modules cache (via PyImport_Import()).

(cherry picked from commit 127c1d2, AKA gh-120224)
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@neonene @mrahtz
pythongh-71587: Drop local reference cache to _strptime module in `… …
1c35bd9 
…_datetime` (pythongh-120224)

The _strptime module object was cached in a static local variable (in the datetime.strptime() implementation).  That's a problem when it crosses isolation boundaries, such as reinitializing the runtme or between interpreters.  This change fixes the problem by dropping the static variable, instead always relying on the normal sys.modules cache (via PyImport_Import()).
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@neonene @noahbkim
pythongh-71587: Drop local reference cache to _strptime module in `… …
d4ec8c3 
…_datetime` (pythongh-120224)

The _strptime module object was cached in a static local variable (in the datetime.strptime() implementation).  That's a problem when it crosses isolation boundaries, such as reinitializing the runtme or between interpreters.  This change fixes the problem by dropping the static variable, instead always relying on the normal sys.modules cache (via PyImport_Import()).
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@neonene @estyxx
pythongh-71587: Drop local reference cache to _strptime module in `… …
1fec6f4 
…_datetime` (pythongh-120224)

The _strptime module object was cached in a static local variable (in the datetime.strptime() implementation).  That's a problem when it crosses isolation boundaries, such as reinitializing the runtme or between interpreters.  This change fixes the problem by dropping the static variable, instead always relying on the normal sys.modules cache (via PyImport_Import()).
@maurycy maurycy mentioned this pull request Apr 4, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin

Assignees

@ericsnowcurrently ericsnowcurrently

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@neonene @ericsnowcurrently @erlend-aasland @kumaraditya303