Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 8ba4df9

Browse filesBrowse files
ericsnowcurrentlyericsnowcurrently
authored
gh-105699: Use a _Py_hashtable_t for the PyModuleDef Cache (gh-106974)
This fixes a crasher due to a race condition, triggered infrequently when two isolated (own GIL) subinterpreters simultaneously initialize their sys or builtins modules. The crash happened due the combination of the "detached" thread state we were using and the "last holder" logic we use for the GIL. It turns out it's tricky to use the same thread state for different threads. Who could have guessed? We solve the problem by eliminating the one object we were still sharing between interpreters. We replace it with a low-level hashtable, using the "raw" allocator to avoid tying it to the main interpreter. We also remove the accommodations for "detached" thread states, which were a dubious idea to start with.
1 parent 55ed85e commit 8ba4df9
Copy full SHA for 8ba4df9

File tree

Expand file treeCollapse file tree

5 files changed

+126
-188
lines changed
Filter options
Expand file treeCollapse file tree

5 files changed

+126
-188
lines changed

‎Include/internal/pycore_import.h

Copy file name to clipboardExpand all lines: Include/internal/pycore_import.h
+4-7Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
extern "C" {
66
#endif
77

8+
#include "pycore_hashtable.h" // _Py_hashtable_t
89
#include "pycore_time.h" // _PyTime_t
910

1011
extern int _PyImport_IsInitialized(PyInterpreterState *);
@@ -36,19 +37,15 @@ struct _import_runtime_state {
3637
See PyInterpreterState.modules_by_index for more info. */
3738
Py_ssize_t last_module_index;
3839
struct {
39-
/* A thread state tied to the main interpreter,
40-
used exclusively for when the extensions dict is access/modified
41-
from an arbitrary thread. */
42-
PyThreadState main_tstate;
43-
/* A lock to guard the dict. */
40+
/* A lock to guard the cache. */
4441
PyThread_type_lock mutex;
45-
/* A dict mapping (filename, name) to PyModuleDef for modules.
42+
/* The actual cache of (filename, name, PyModuleDef) for modules.
4643
Only legacy (single-phase init) extension modules are added
4744
and only if they support multiple initialization (m_size >- 0)
4845
or are imported in the main interpreter.
4946
This is initialized lazily in _PyImport_FixupExtensionObject().
5047
Modules are added there and looked up in _imp.find_extension(). */
51-
PyObject *dict;
48+
_Py_hashtable_t *hashtable;
5249
} extensions;
5350
/* Package context -- the full module name for package imports */
5451
const char * pkgcontext;

‎Include/internal/pycore_pystate.h

Copy file name to clipboardExpand all lines: Include/internal/pycore_pystate.h
-5Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,6 @@ extern PyThreadState * _PyThreadState_New(PyInterpreterState *interp);
125125
extern void _PyThreadState_Bind(PyThreadState *tstate);
126126
extern void _PyThreadState_DeleteExcept(PyThreadState *tstate);
127127

128-
extern void _PyThreadState_InitDetached(PyThreadState *, PyInterpreterState *);
129-
extern void _PyThreadState_ClearDetached(PyThreadState *);
130-
extern void _PyThreadState_BindDetached(PyThreadState *);
131-
extern void _PyThreadState_UnbindDetached(PyThreadState *);
132-
133128
// Export for '_testinternalcapi' shared extension
134129
PyAPI_FUNC(PyObject*) _PyThreadState_GetDict(PyThreadState *tstate);
135130

‎Include/internal/pycore_runtime_init.h

Copy file name to clipboardExpand all lines: Include/internal/pycore_runtime_init.h
-5Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,6 @@ extern PyTypeObject _PyExc_MemoryError;
9797
in accordance with the specification. */ \
9898
.autoTSSkey = Py_tss_NEEDS_INIT, \
9999
.parser = _parser_runtime_state_INIT, \
100-
.imports = { \
101-
.extensions = { \
102-
.main_tstate = _PyThreadState_INIT, \
103-
}, \
104-
}, \
105100
.ceval = { \
106101
.perf = _PyEval_RUNTIME_PERF_INIT, \
107102
}, \

‎Python/import.c

Copy file name to clipboardExpand all lines: Python/import.c
+122-102Lines changed: 122 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@
22

33
#include "Python.h"
44

5+
#include "pycore_hashtable.h" // _Py_hashtable_new_full()
56
#include "pycore_import.h" // _PyImport_BootstrapImp()
67
#include "pycore_initconfig.h" // _PyStatus_OK()
78
#include "pycore_interp.h" // struct _import_runtime_state
89
#include "pycore_namespace.h" // _PyNamespace_Type
10+
#include "pycore_object.h" // _Py_SetImmortal()
911
#include "pycore_pyerrors.h" // _PyErr_SetString()
1012
#include "pycore_pyhash.h" // _Py_KeyedHash()
1113
#include "pycore_pylifecycle.h"
@@ -905,172 +907,192 @@ extensions_lock_release(void)
905907
dictionary, to avoid loading shared libraries twice.
906908
*/
907909

910+
static void *
911+
hashtable_key_from_2_strings(PyObject *str1, PyObject *str2, const char sep)
912+
{
913+
Py_ssize_t str1_len, str2_len;
914+
const char *str1_data = PyUnicode_AsUTF8AndSize(str1, &str1_len);
915+
const char *str2_data = PyUnicode_AsUTF8AndSize(str2, &str2_len);
916+
if (str1_data == NULL || str2_data == NULL) {
917+
return NULL;
918+
}
919+
/* Make sure sep and the NULL byte won't cause an overflow. */
920+
assert(SIZE_MAX - str1_len - str2_len > 2);
921+
size_t size = str1_len + 1 + str2_len + 1;
922+
923+
char *key = PyMem_RawMalloc(size);
924+
if (key == NULL) {
925+
PyErr_NoMemory();
926+
return NULL;
927+
}
928+
929+
strncpy(key, str1_data, str1_len);
930+
key[str1_len] = sep;
931+
strncpy(key + str1_len + 1, str2_data, str2_len + 1);
932+
assert(strlen(key) == size - 1);
933+
return key;
934+
}
935+
936+
static Py_uhash_t
937+
hashtable_hash_str(const void *key)
938+
{
939+
return _Py_HashBytes(key, strlen((const char *)key));
940+
}
941+
942+
static int
943+
hashtable_compare_str(const void *key1, const void *key2)
944+
{
945+
return strcmp((const char *)key1, (const char *)key2) == 0;
946+
}
947+
908948
static void
909-
_extensions_cache_init(void)
949+
hashtable_destroy_str(void *ptr)
910950
{
911-
/* The runtime (i.e. main interpreter) must be initializing,
912-
so we don't need to worry about the lock. */
913-
_PyThreadState_InitDetached(&EXTENSIONS.main_tstate,
914-
_PyInterpreterState_Main());
951+
PyMem_RawFree(ptr);
915952
}
916953

954+
#define HTSEP ':'
955+
917956
static PyModuleDef *
918957
_extensions_cache_get(PyObject *filename, PyObject *name)
919958
{
920959
PyModuleDef *def = NULL;
960+
void *key = NULL;
921961
extensions_lock_acquire();
922962

923-
PyObject *key = PyTuple_Pack(2, filename, name);
924-
if (key == NULL) {
963+
if (EXTENSIONS.hashtable == NULL) {
925964
goto finally;
926965
}
927966

928-
PyObject *extensions = EXTENSIONS.dict;
929-
if (extensions == NULL) {
967+
key = hashtable_key_from_2_strings(filename, name, HTSEP);
968+
if (key == NULL) {
969+
goto finally;
970+
}
971+
_Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(
972+
EXTENSIONS.hashtable, key);
973+
if (entry == NULL) {
930974
goto finally;
931975
}
932-
def = (PyModuleDef *)PyDict_GetItemWithError(extensions, key);
976+
def = (PyModuleDef *)entry->value;
933977

934978
finally:
935-
Py_XDECREF(key);
936979
extensions_lock_release();
980+
if (key != NULL) {
981+
PyMem_RawFree(key);
982+
}
937983
return def;
938984
}
939985

940986
static int
941987
_extensions_cache_set(PyObject *filename, PyObject *name, PyModuleDef *def)
942988
{
943989
int res = -1;
944-
PyThreadState *oldts = NULL;
945990
extensions_lock_acquire();
946991

947-
/* Swap to the main interpreter, if necessary. This matters if
948-
the dict hasn't been created yet or if the item isn't in the
949-
dict yet. In both cases we must ensure the relevant objects
950-
are created using the main interpreter. */
951-
PyThreadState *main_tstate = &EXTENSIONS.main_tstate;
952-
PyInterpreterState *interp = _PyInterpreterState_GET();
953-
if (!_Py_IsMainInterpreter(interp)) {
954-
_PyThreadState_BindDetached(main_tstate);
955-
oldts = _PyThreadState_Swap(interp->runtime, main_tstate);
956-
assert(!_Py_IsMainInterpreter(oldts->interp));
957-
958-
/* Make sure the name and filename objects are owned
959-
by the main interpreter. */
960-
name = PyUnicode_InternFromString(PyUnicode_AsUTF8(name));
961-
assert(name != NULL);
962-
filename = PyUnicode_InternFromString(PyUnicode_AsUTF8(filename));
963-
assert(filename != NULL);
992+
if (EXTENSIONS.hashtable == NULL) {
993+
_Py_hashtable_allocator_t alloc = {PyMem_RawMalloc, PyMem_RawFree};
994+
EXTENSIONS.hashtable = _Py_hashtable_new_full(
995+
hashtable_hash_str,
996+
hashtable_compare_str,
997+
hashtable_destroy_str, // key
998+
/* There's no need to decref the def since it's immortal. */
999+
NULL, // value
1000+
&alloc
1001+
);
1002+
if (EXTENSIONS.hashtable == NULL) {
1003+
PyErr_NoMemory();
1004+
goto finally;
1005+
}
9641006
}
9651007

966-
PyObject *key = PyTuple_Pack(2, filename, name);
1008+
void *key = hashtable_key_from_2_strings(filename, name, HTSEP);
9671009
if (key == NULL) {
9681010
goto finally;
9691011
}
9701012

971-
PyObject *extensions = EXTENSIONS.dict;
972-
if (extensions == NULL) {
973-
extensions = PyDict_New();
974-
if (extensions == NULL) {
1013+
int already_set = 0;
1014+
_Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(
1015+
EXTENSIONS.hashtable, key);
1016+
if (entry == NULL) {
1017+
if (_Py_hashtable_set(EXTENSIONS.hashtable, key, def) < 0) {
1018+
PyMem_RawFree(key);
1019+
PyErr_NoMemory();
9751020
goto finally;
9761021
}
977-
EXTENSIONS.dict = extensions;
978-
}
979-
980-
PyModuleDef *actual = (PyModuleDef *)PyDict_GetItemWithError(extensions, key);
981-
if (PyErr_Occurred()) {
982-
goto finally;
9831022
}
984-
else if (actual != NULL) {
985-
/* We expect it to be static, so it must be the same pointer. */
986-
assert(def == actual);
987-
res = 0;
988-
goto finally;
1023+
else {
1024+
if (entry->value == NULL) {
1025+
entry->value = def;
1026+
}
1027+
else {
1028+
/* We expect it to be static, so it must be the same pointer. */
1029+
assert((PyModuleDef *)entry->value == def);
1030+
already_set = 1;
1031+
}
1032+
PyMem_RawFree(key);
9891033
}
990-
991-
/* This might trigger a resize, which is why we must switch
992-
to the main interpreter. */
993-
res = PyDict_SetItem(extensions, key, (PyObject *)def);
994-
if (res < 0) {
995-
res = -1;
996-
goto finally;
1034+
if (!already_set) {
1035+
/* We assume that all module defs are statically allocated
1036+
and will never be freed. Otherwise, we would incref here. */
1037+
_Py_SetImmortal(def);
9971038
}
9981039
res = 0;
9991040

10001041
finally:
1001-
Py_XDECREF(key);
1002-
if (oldts != NULL) {
1003-
_PyThreadState_Swap(interp->runtime, oldts);
1004-
_PyThreadState_UnbindDetached(main_tstate);
1005-
Py_DECREF(name);
1006-
Py_DECREF(filename);
1007-
}
10081042
extensions_lock_release();
10091043
return res;
10101044
}
10111045

1012-
static int
1046+
static void
10131047
_extensions_cache_delete(PyObject *filename, PyObject *name)
10141048
{
1015-
int res = -1;
1016-
PyThreadState *oldts = NULL;
1049+
void *key = NULL;
10171050
extensions_lock_acquire();
10181051

1019-
PyObject *key = PyTuple_Pack(2, filename, name);
1020-
if (key == NULL) {
1052+
if (EXTENSIONS.hashtable == NULL) {
1053+
/* It was never added. */
10211054
goto finally;
10221055
}
10231056

1024-
PyObject *extensions = EXTENSIONS.dict;
1025-
if (extensions == NULL) {
1026-
res = 0;
1057+
key = hashtable_key_from_2_strings(filename, name, HTSEP);
1058+
if (key == NULL) {
10271059
goto finally;
10281060
}
10291061

1030-
PyModuleDef *actual = (PyModuleDef *)PyDict_GetItemWithError(extensions, key);
1031-
if (PyErr_Occurred()) {
1062+
_Py_hashtable_entry_t *entry = _Py_hashtable_get_entry(
1063+
EXTENSIONS.hashtable, key);
1064+
if (entry == NULL) {
1065+
/* It was never added. */
10321066
goto finally;
10331067
}
1034-
else if (actual == NULL) {
1035-
/* It was already removed or never added. */
1036-
res = 0;
1068+
if (entry->value == NULL) {
1069+
/* It was already removed. */
10371070
goto finally;
10381071
}
1039-
1040-
/* Swap to the main interpreter, if necessary. */
1041-
PyThreadState *main_tstate = &EXTENSIONS.main_tstate;
1042-
PyInterpreterState *interp = _PyInterpreterState_GET();
1043-
if (!_Py_IsMainInterpreter(interp)) {
1044-
_PyThreadState_BindDetached(main_tstate);
1045-
oldts = _PyThreadState_Swap(interp->runtime, main_tstate);
1046-
assert(!_Py_IsMainInterpreter(oldts->interp));
1047-
}
1048-
1049-
if (PyDict_DelItem(extensions, key) < 0) {
1050-
goto finally;
1051-
}
1052-
res = 0;
1072+
/* If we hadn't made the stored defs immortal, we would decref here.
1073+
However, this decref would be problematic if the module def were
1074+
dynamically allocated, it were the last ref, and this function
1075+
were called with an interpreter other than the def's owner. */
1076+
entry->value = NULL;
10531077

10541078
finally:
1055-
if (oldts != NULL) {
1056-
_PyThreadState_Swap(interp->runtime, oldts);
1057-
_PyThreadState_UnbindDetached(main_tstate);
1058-
}
1059-
Py_XDECREF(key);
10601079
extensions_lock_release();
1061-
return res;
1080+
if (key != NULL) {
1081+
PyMem_RawFree(key);
1082+
}
10621083
}
10631084

10641085
static void
10651086
_extensions_cache_clear_all(void)
10661087
{
10671088
/* The runtime (i.e. main interpreter) must be finalizing,
10681089
so we don't need to worry about the lock. */
1069-
// XXX assert(_Py_IsMainInterpreter(_PyInterpreterState_GET()));
1070-
Py_CLEAR(EXTENSIONS.dict);
1071-
_PyThreadState_ClearDetached(&EXTENSIONS.main_tstate);
1090+
_Py_hashtable_destroy(EXTENSIONS.hashtable);
1091+
EXTENSIONS.hashtable = NULL;
10721092
}
10731093

1094+
#undef HTSEP
1095+
10741096

10751097
static bool
10761098
check_multi_interp_extensions(PyInterpreterState *interp)
@@ -1231,6 +1253,8 @@ import_find_extension(PyThreadState *tstate, PyObject *name,
12311253
PyObject *m_copy = def->m_base.m_copy;
12321254
/* Module does not support repeated initialization */
12331255
if (m_copy == NULL) {
1256+
/* It might be a core module (e.g. sys & builtins),
1257+
for which we don't set m_copy. */
12341258
m_copy = get_core_module_dict(tstate->interp, name, filename);
12351259
if (m_copy == NULL) {
12361260
return NULL;
@@ -1300,9 +1324,7 @@ clear_singlephase_extension(PyInterpreterState *interp,
13001324
}
13011325

13021326
/* Clear the cached module def. */
1303-
if (_extensions_cache_delete(filename, name) < 0) {
1304-
return -1;
1305-
}
1327+
_extensions_cache_delete(filename, name);
13061328

13071329
return 0;
13081330
}
@@ -3051,6 +3073,8 @@ void
30513073
_PyImport_Fini(void)
30523074
{
30533075
/* Destroy the database used by _PyImport_{Fixup,Find}Extension */
3076+
// XXX Should we actually leave them (mostly) intact, since we don't
3077+
// ever dlclose() the module files?
30543078
_extensions_cache_clear_all();
30553079

30563080
/* Use the same memory allocator as _PyImport_Init(). */
@@ -3088,10 +3112,6 @@ _PyImport_Fini2(void)
30883112
PyStatus
30893113
_PyImport_InitCore(PyThreadState *tstate, PyObject *sysmod, int importlib)
30903114
{
3091-
if (_Py_IsMainInterpreter(tstate->interp)) {
3092-
_extensions_cache_init();
3093-
}
3094-
30953115
// XXX Initialize here: interp->modules and interp->import_func.
30963116
// XXX Initialize here: sys.modules and sys.meta_path.
30973117

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.