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
contextvars: hamt_alloc() must initialize h_root and h_count fields #77984
Comments
test_asyncio started to crash in #7487 I debugged the crash with Yury: _PyHAMT_New() triggers indirectly a GC collection at "o->h_root = hamt_node_bitmap_new(0);". Problem: the object that is being created is already tracked by the GC, whereas its h_root field is not set. Then the GC does crash because the field is a random pointer. hamt_alloc() must initialize h_root and h_count before tracking the object in the GC. Thinking about the GC when writing an object constructor is hard :-( Maybe we should add a debug option to trigger the GC more often to make such random bug more likely. contextvars has been implemented a few months ago, and we only spotted the bug a few days before Python 3.7.0 final release... |
One solution to trigger so crash more frequently is to reduce the threshold of the generation 0 of the garbage collector. Here is a patch to do that when using -X dev: change the default threshold from 700 to 5 for the generation 0. With this patch, "PYTHONDEVMODE=1 python -m test test_context" does also crash as expected. diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c
index 958219b744..81218fb964 100644
--- a/Python/pylifecycle.c
+++ b/Python/pylifecycle.c
@@ -622,6 +622,10 @@ _Py_InitializeCore(const _PyCoreConfig *core_config)
return _Py_INIT_ERR("runtime core already initialized");
}
+ if (core_config->dev_mode) {
+ _PyRuntime.gc.generations[0].threshold = 5;
+ }
+
/* Py_Finalize leaves _Py_Finalizing set in order to help daemon
* threads behave a little more gracefully at interpreter shutdown.
* We clobber it here so the new interpreter can start with a clean |
Don't forget to merge #7487 once this bug is fixed. I would like to see https://bugs.python.org/issue33792 in Python 3.7 *if possible* (the feature now seems "required" for the new asyncio.loop() function). |
Thanks Victor for debugging this. I made a PR (which is now trivial) and double checked all other calls to GCTrack in context.c & hamt.c. |
I'd love to have a flag to turn this on, or maybe we should enable it for -X dev. |
Well, there is already a public API to do it manually: gc.set_threshold(5). I'm not sure about a threshold of 5 for -X dev: that's very very low and so kill performances. -X dev can be slower, not but 10x slower for example. I didn't measure performance. Such bug is rare, so I'm not sure about putting gc.set_threshold(5) in -X dev. |
FYI the bug was also seen 8 hours ago on a different asyncio PR: |
Oops, my message is misleading: the crash is not a regression. I just wanted to notice that a different PR also triggered the crash before the crash has been fixed. I'm just surprised that the bug decided to wake up 4 months after contextvars has been merged. Why yesterday and not previously? |
Notes for myself. In bpo-38392, I modified PyObject_GC_Track() in debug mode to detect this bug. For example, this bug can be reproduced with this change: diff --git a/Python/hamt.c b/Python/hamt.c
index 28b4e1ef6c..d7dd555d15 100644
--- a/Python/hamt.c
+++ b/Python/hamt.c
@@ -2478,8 +2478,6 @@ hamt_alloc(void)
if (o == NULL) {
return NULL;
}
- o->h_count = 0;
- o->h_root = NULL;
o->h_weakreflist = NULL;
PyObject_GC_Track(o);
return o; Python now detects the bug in debug mode: $ ./python -m test -v test_context
(...)
test_context_copy_1 (test.test_context.ContextTest) ...
Modules/gcmodule.c:1931: visit_validate: Assertion failed: PyObject_GC_Track() object is not valid
Enable tracemalloc to get the memory block allocation traceback object address : 0x7f2b17408d70 Fatal Python error: _PyObject_AssertFailed Current thread 0x00007f2b25590740 (most recent call first): |
Awesome!!! |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: