Skip to content
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

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

Merged
merged 10 commits into from
Jun 12, 2024
Merged

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

merged 10 commits into from
Jun 12, 2024

Conversation

neonene
Copy link
Contributor

@neonene neonene commented Jun 7, 2024

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


Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
neonene and others added 2 commits June 7, 2024 22:50
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Modules/_datetimemodule.c Show resolved Hide resolved
Modules/_datetimemodule.c Outdated Show resolved Hide resolved
@neonene neonene changed the title gh-71587: Fix reference caching to _strptime module in _datetime gh-71587: Drop local reference cache to _strptime module in _datetime Jun 7, 2024
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

@neonene
Copy link
Contributor Author

neonene commented Jun 9, 2024

@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?

@ericsnowcurrently
Copy link
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.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

Modules/_datetimemodule.c Show resolved Hide resolved
@ericsnowcurrently
Copy link
Member

This should be backported to 3.13, right?

@neonene
Copy link
Contributor Author

neonene commented Jun 12, 2024

I agree to the 3.13 backport.

@ericsnowcurrently ericsnowcurrently added the needs backport to 3.13 bugs and security fixes label Jun 12, 2024
@ericsnowcurrently ericsnowcurrently merged commit 127c1d2 into python:main Jun 12, 2024
37 checks passed
@miss-islington-app
Copy link

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
…_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
Copy link

bedevere-app bot commented Jun 12, 2024

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
Copy link
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 bug and security fixes label Jun 12, 2024
@miss-islington-app
Copy link

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

@miss-islington-app
Copy link

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
Copy link
Member

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

ericsnowcurrently pushed a commit that referenced this pull request Jun 12, 2024
…`_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
Copy link

bedevere-app bot commented Jun 12, 2024

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

@neonene
Copy link
Contributor Author

neonene commented Jun 12, 2024

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
…`_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
…_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
…_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
…_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()).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants