gh-120161: Fix a Crash in the _datetime Module - #120182
ericsnowcurrently merged 7 commits into
Conversation
Sorry, something went wrong.
|
FYI, I'm planning on merging this Friday, barring any feedback. |
Sorry, something went wrong.
|
|
||
| with self.subTest('complex'): | ||
| script = textwrap.dedent(""" | ||
| import asyncio |
There was a problem hiding this comment.
I would separate this test into its own method, it imports asyncio. Which means that it needs @requires_working_socket() decorator
Sorry, something went wrong.
There was a problem hiding this comment.
Btw, why does it need asyncio? 🤔
Sorry, something went wrong.
There was a problem hiding this comment.
Because of the original issue (the snippet is taken almost verbatim).
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| self.assertEqual(dt_orig, dt_rt) | ||
|
|
||
|
|
||
| class ExtensionModuleTests(unittest.TestCase): |
There was a problem hiding this comment.
@cpython_only?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
Thanks @ericsnowcurrently for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
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>
|
GH-120518 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
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>
|
Looks like this broke some buildbots:
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) |
Sorry, something went wrong.
…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>
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.
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.
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.
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.