gh-120161: Fix a Crash in the _datetime Module #120182

ericsnowcurrently · 2024-06-06T23:34:45Z

In gh-120009 I used an atexit hook to finalize the _datetime module's static types at interpreter shutdown. However, atexit hooks are executed very early in finalization, which is a problem in the few cases where a subclass of one of those static types is still alive until the final GC collection. The static builtin types don't have this probably because they are finalized toward the end, after the final GC collection. To avoid the problem for _datetime, I applied a similar approach here.

Also, credit goes to @mgorny and @neonene for the new tests.

FYI, I'd like to take a slightly cleaner approach with managed static types, but wanted to get a smaller fix in first for the sake of backporting. I'll circle back to the cleaner approach on main after this is merged.

Issue: Objects/typeobject.c:143: managed_static_type_index_get: Assertion managed_static_type_index_is_set(self)' failed.` in 3.13.0b2+, with freezegun #120161

ericsnowcurrently · 2024-06-12T17:35:43Z

FYI, I'm planning on merging this Friday, barring any feedback.

sobolevn · 2024-06-13T06:50:19Z

Lib/test/datetimetester.py

+
+        with self.subTest('complex'):
+            script = textwrap.dedent("""
+                import asyncio


I would separate this test into its own method, it imports asyncio. Which means that it needs @requires_working_socket() decorator

Btw, why does it need asyncio? 🤔

Because of the original issue (the snippet is taken almost verbatim).

Importing asyncio on its own is not enough to be a problem. I was able to verify that it works fine on a WASI build (where @requires_working_socket() would skip). This is also verified by the WASI CI check passing. Unless there is another reason to do so, I'd rather not add that decorator.

sobolevn · 2024-06-13T06:51:28Z

Lib/test/datetimetester.py

@@ -6781,6 +6782,47 @@ def test_datetime_from_timestamp(self):
                    self.assertEqual(dt_orig, dt_rt)


+class ExtensionModuleTests(unittest.TestCase):


@cpython_only?

We could, but currently there's mostly no need. If the extension module ("_datetime") doesn't exist then the "fast" version of the test is never created, and the pure-Python version of the test already always skips. (See Lib/test/test_datetime.py.) Also note that @cpython_only would mean the test wouldn't be run for Python implementations that do provide a _datetime accelerator module.

All that said, I don't see any harm with adding @cpython_only on the new test method. I would certainly only expect it to fail on CPython. Thanks for bringing this up.

miss-islington-app · 2024-06-14T19:29:13Z

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

@mgorny

In pythongh-120009 I used an atexit hook to finalize the _datetime module's static types at interpreter shutdown. However, atexit hooks are executed very early in finalization, which is a problem in the few cases where a subclass of one of those static types is still alive until the final GC collection. The static builtin types don't have this probably because they are finalized toward the end, after the final GC collection. To avoid the problem for _datetime, I have applied a similar approach here. Also, credit goes to @mgorny and @neonene for the new tests. FYI, I would have liked to take a slightly cleaner approach with managed static types, but wanted to get a smaller fix in first for the sake of backporting. I'll circle back to the cleaner approach with a future change on the main branch. (cherry picked from commit b2e71ff) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

bedevere-app · 2024-06-14T19:29:24Z

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

@mgorny

In gh-120009 I used an atexit hook to finalize the _datetime module's static types at interpreter shutdown. However, atexit hooks are executed very early in finalization, which is a problem in the few cases where a subclass of one of those static types is still alive until the final GC collection. The static builtin types don't have this probably because they are finalized toward the end, after the final GC collection. To avoid the problem for _datetime, I have applied a similar approach here. Also, credit goes to @mgorny and @neonene for the new tests. FYI, I would have liked to take a slightly cleaner approach with managed static types, but wanted to get a smaller fix in first for the sake of backporting. I'll circle back to the cleaner approach with a future change on the main branch. (cherry picked from commit b2e71ff, AKA gh-120182) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

ericsnowcurrently · 2024-06-14T21:16:38Z

Looks like this broke some buildbots:

Arm64 MacOS: https://buildbot.python.org/all/#/builders/725/builds/8270
AMD64 Windows 10 (3.13): https://buildbot.python.org/all/#/builders/1501/builds/172
...

test_create_many_threaded (test.test_interpreters.test_stress.StressTests.test_create_many_threaded) ... python: Objects/typeobject.c:248: managed_static_type_state_init: Assertion `(initial == 1) == (_PyRuntime.types.managed_static.types[full_index].interp_count == 0)' failed.
Aborted (core dumped)

ericsnowcurrently · 2024-06-14T21:42:22Z

See gh-120524.

…nterp_count (gh-120529) gh-120182 added new global state (interp_count), but didn't add thread-safety for it. This change eliminates the possible race.

…s[i].interp_count (pythongh-120529) pythongh-120182 added new global state (interp_count), but didn't add thread-safety for it. This change eliminates the possible race. (cherry picked from commit 2c66318) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

…es[i].interp_count (gh-120657) gh-120182 added new global state (interp_count), but didn't add thread-safety for it. This change eliminates the possible race. (cherry picked from commit 2c66318, AKA gh-120529) Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>

@mgorny

In pythongh-120009 I used an atexit hook to finalize the _datetime module's static types at interpreter shutdown. However, atexit hooks are executed very early in finalization, which is a problem in the few cases where a subclass of one of those static types is still alive until the final GC collection. The static builtin types don't have this probably because they are finalized toward the end, after the final GC collection. To avoid the problem for _datetime, I have applied a similar approach here. Also, credit goes to @mgorny and @neonene for the new tests. FYI, I would have liked to take a slightly cleaner approach with managed static types, but wanted to get a smaller fix in first for the sake of backporting. I'll circle back to the cleaner approach with a future change on the main branch.

…s[i].interp_count (pythongh-120529) pythongh-120182 added new global state (interp_count), but didn't add thread-safety for it. This change eliminates the possible race.

@mgorny

In pythongh-120009 I used an atexit hook to finalize the _datetime module's static types at interpreter shutdown. However, atexit hooks are executed very early in finalization, which is a problem in the few cases where a subclass of one of those static types is still alive until the final GC collection. The static builtin types don't have this probably because they are finalized toward the end, after the final GC collection. To avoid the problem for _datetime, I have applied a similar approach here. Also, credit goes to @mgorny and @neonene for the new tests. FYI, I would have liked to take a slightly cleaner approach with managed static types, but wanted to get a smaller fix in first for the sake of backporting. I'll circle back to the cleaner approach with a future change on the main branch.

…s[i].interp_count (pythongh-120529) pythongh-120182 added new global state (interp_count), but didn't add thread-safety for it. This change eliminates the possible race.

@mgorny

In pythongh-120009 I used an atexit hook to finalize the _datetime module's static types at interpreter shutdown. However, atexit hooks are executed very early in finalization, which is a problem in the few cases where a subclass of one of those static types is still alive until the final GC collection. The static builtin types don't have this probably because they are finalized toward the end, after the final GC collection. To avoid the problem for _datetime, I have applied a similar approach here. Also, credit goes to @mgorny and @neonene for the new tests. FYI, I would have liked to take a slightly cleaner approach with managed static types, but wanted to get a smaller fix in first for the sake of backporting. I'll circle back to the cleaner approach with a future change on the main branch.

…s[i].interp_count (pythongh-120529) pythongh-120182 added new global state (interp_count), but didn't add thread-safety for it. This change eliminates the possible race.

ericsnowcurrently added 4 commits June 6, 2024 17:21

Add a test.

1cb0da2

Add _PyTypes_FiniExtTypes().

0054962

Drop unnecessary code.

7d8a19f

Add a NEWS entry.

34d17e1

ericsnowcurrently added the needs backport to 3.13 bugs and security fixes label Jun 6, 2024

ericsnowcurrently requested review from sobolevn and erlend-aasland June 6, 2024 23:34

ericsnowcurrently requested review from pganssle, abalkin and markshannon as code owners June 6, 2024 23:34

bedevere-app bot added the awaiting core review label Jun 6, 2024

bedevere-app bot mentioned this pull request Jun 6, 2024

Objects/typeobject.c:143: managed_static_type_index_get: Assertion managed_static_type_index_is_set(self)' failed.` in 3.13.0b2+, with freezegun #120161

Closed

ericsnowcurrently removed request for abalkin and markshannon June 6, 2024 23:35

ericsnowcurrently added 2 commits June 12, 2024 10:30

Add a simpler test.

a7d2f06

Merge branch 'main' into fix-datetime-creash-pythongh-120161

99db91f

sobolevn reviewed Jun 13, 2024

View reviewed changes

Only run the new test for CPython.

3c0084d

ericsnowcurrently merged commit b2e71ff into python:main Jun 14, 2024
36 checks passed

bedevere-app bot removed the awaiting core review label Jun 14, 2024

ericsnowcurrently deleted the fix-datetime-creash-gh-120161 branch June 14, 2024 19:29

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 14, 2024

This was referenced Jun 14, 2024

[3.13] gh-120161: Fix a Crash in the _datetime Module (gh-120182) #120518

Merged

test_create_many_threaded() of test_interpreters.test_stress Hitting an Assert #120524

Closed

ericsnowcurrently mentioned this pull request Jun 14, 2024

gh-120524: Avoid a Race On _PyRuntime.types.managed_static.types[i].interp_count #120529

Merged

miss-islington mentioned this pull request Jun 17, 2024

[3.13] gh-120524: Avoid a Race On _PyRuntime.types.managed_static.types[i].interp_count (gh-120529) #120657

Merged

ericsnowcurrently mentioned this pull request Jun 17, 2024

gh-120161: Clean Up the Managed Static Types State #120663

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-120161: Fix a Crash in the _datetime Module #120182

gh-120161: Fix a Crash in the _datetime Module #120182

ericsnowcurrently commented Jun 6, 2024 •

edited

Loading

ericsnowcurrently commented Jun 12, 2024

sobolevn Jun 13, 2024

sobolevn Jun 13, 2024

picnixz Jun 13, 2024

ericsnowcurrently Jun 13, 2024

sobolevn Jun 13, 2024

ericsnowcurrently Jun 13, 2024

miss-islington-app bot commented Jun 14, 2024

bedevere-app bot commented Jun 14, 2024

ericsnowcurrently commented Jun 14, 2024

ericsnowcurrently commented Jun 14, 2024

		@@ -6781,6 +6782,47 @@ def test_datetime_from_timestamp(self):
		self.assertEqual(dt_orig, dt_rt)


		class ExtensionModuleTests(unittest.TestCase):

gh-120161: Fix a Crash in the _datetime Module #120182

gh-120161: Fix a Crash in the _datetime Module #120182

Conversation

ericsnowcurrently commented Jun 6, 2024 • edited Loading

ericsnowcurrently commented Jun 12, 2024

sobolevn Jun 13, 2024

Choose a reason for hiding this comment

sobolevn Jun 13, 2024

Choose a reason for hiding this comment

picnixz Jun 13, 2024

Choose a reason for hiding this comment

ericsnowcurrently Jun 13, 2024

Choose a reason for hiding this comment

sobolevn Jun 13, 2024

Choose a reason for hiding this comment

ericsnowcurrently Jun 13, 2024

Choose a reason for hiding this comment

miss-islington-app bot commented Jun 14, 2024

bedevere-app bot commented Jun 14, 2024

ericsnowcurrently commented Jun 14, 2024

ericsnowcurrently commented Jun 14, 2024

ericsnowcurrently commented Jun 6, 2024 •

edited

Loading