Skip to content

Commit

Permalink
Ensure objects found by gc.get_objects() are kept alive.
Browse files Browse the repository at this point in the history
After the `_PyEval_StartTheWorld()` call, other threads may be running
and mutating objects. Ensure that the objects are kept alive by
incref'ing them when they're added to the `_PyObjectStack`.
  • Loading branch information
colesbury committed Oct 23, 2024
1 parent aa21b07 commit b6b9b7a
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 40 deletions.
10 changes: 10 additions & 0 deletions Include/internal/pycore_object_stack.h
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,16 @@ _PyObjectStack_Pop(_PyObjectStack *stack)
return obj;
}

static inline Py_ssize_t
_PyObjectStack_Size(_PyObjectStack *stack)
{
Py_ssize_t size = 0;
for (_PyObjectStackChunk *buf = stack->head; buf != NULL; buf = buf->prev) {
size += buf->n;
}
return size;
}

// Merge src into dst, leaving src empty
extern void
_PyObjectStack_Merge(_PyObjectStack *dst, _PyObjectStack *src);
Expand Down
61 changes: 61 additions & 0 deletions Lib/test/test_free_threading/test_gc.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import unittest

import threading
from threading import Thread
from unittest import TestCase
import gc

from test.support import threading_helper


class MyObj:
pass


@threading_helper.requires_working_threading()
class TestGC(TestCase):
def test_get_objects(self):
event = threading.Event()

def gc_thread():
for i in range(100):
o = gc.get_objects()
event.set()

def mutator_thread():
while not event.is_set():
o1 = MyObj()
o2 = MyObj()
o3 = MyObj()
o4 = MyObj()

gcs = [Thread(target=gc_thread)]
mutators = [Thread(target=mutator_thread) for _ in range(4)]
with threading_helper.start_threads(gcs + mutators):
pass

def test_get_referrers(self):
event = threading.Event()

obj = MyObj()

def gc_thread():
for i in range(100):
o = gc.get_referrers(obj)
event.set()

def mutator_thread():
while not event.is_set():
d1 = { "key": obj }
d2 = { "key": obj }
d3 = { "key": obj }
d4 = { "key": obj }

gcs = [Thread(target=gc_thread) for _ in range(2)]
mutators = [Thread(target=mutator_thread) for _ in range(4)]
with threading_helper.start_threads(gcs + mutators):
pass


if __name__ == "__main__":
unittest.main()
74 changes: 34 additions & 40 deletions Python/gc_free_threading.c
Original file line number Diff line number Diff line change
Expand Up @@ -1401,6 +1401,28 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason)
return n + m;
}

static PyObject *
list_from_object_stack(_PyObjectStack *stack)
{
PyObject *list = PyList_New(_PyObjectStack_Size(stack));
if (list == NULL) {
PyObject *op;
while ((op = _PyObjectStack_Pop(stack)) != NULL) {
Py_DECREF(op);
}
return NULL;
}

PyObject *op;
Py_ssize_t idx = 0;
while ((op = _PyObjectStack_Pop(stack)) != NULL) {
assert(idx < PyList_GET_SIZE(list));
PyList_SET_ITEM(list, idx++, op);
}
assert(idx == PyList_GET_SIZE(list));
return list;
}

struct get_referrers_args {
struct visitor_args base;
PyObject *objs;
Expand Down Expand Up @@ -1435,8 +1457,12 @@ visit_get_referrers(const mi_heap_t *heap, const mi_heap_area_t *area,
}

struct get_referrers_args *arg = (struct get_referrers_args *)args;
if (op == arg->objs) {
// Don't include the tuple itself in the referrers list.
return true;
}
if (Py_TYPE(op)->tp_traverse(op, referrersvisit, arg->objs)) {
if (_PyObjectStack_Push(&arg->results, op) < 0) {
if (_PyObjectStack_Push(&arg->results, Py_NewRef(op)) < 0) {
return false;
}
}
Expand All @@ -1447,11 +1473,6 @@ visit_get_referrers(const mi_heap_t *heap, const mi_heap_area_t *area,
PyObject *
_PyGC_GetReferrers(PyInterpreterState *interp, PyObject *objs)
{
PyObject *result = PyList_New(0);
if (!result) {
return NULL;
}

// NOTE: We can't append to the PyListObject during gc_visit_heaps()
// because PyList_Append() may reclaim an abandoned mimalloc segments
// while we are traversing them.
Expand All @@ -1460,23 +1481,12 @@ _PyGC_GetReferrers(PyInterpreterState *interp, PyObject *objs)
int err = gc_visit_heaps(interp, &visit_get_referrers, &args.base);
_PyEval_StartTheWorld(interp);

PyObject *list = list_from_object_stack(&args.results);
if (err < 0) {
PyErr_NoMemory();
goto error;
Py_CLEAR(list);
}

PyObject *op;
while ((op = _PyObjectStack_Pop(&args.results)) != NULL) {
if (op != objs && PyList_Append(result, op) < 0) {
goto error;
}
}
return result;

error:
Py_DECREF(result);
_PyObjectStack_Clear(&args.results);
return NULL;
return list;
}

struct get_objects_args {
Expand All @@ -1499,7 +1509,7 @@ visit_get_objects(const mi_heap_t *heap, const mi_heap_area_t *area,
}

struct get_objects_args *arg = (struct get_objects_args *)args;
if (_PyObjectStack_Push(&arg->objects, op) < 0) {
if (_PyObjectStack_Push(&arg->objects, Py_NewRef(op)) < 0) {
return false;
}
return true;
Expand All @@ -1508,11 +1518,6 @@ visit_get_objects(const mi_heap_t *heap, const mi_heap_area_t *area,
PyObject *
_PyGC_GetObjects(PyInterpreterState *interp, int generation)
{
PyObject *result = PyList_New(0);
if (!result) {
return NULL;
}

// NOTE: We can't append to the PyListObject during gc_visit_heaps()
// because PyList_Append() may reclaim an abandoned mimalloc segments
// while we are traversing them.
Expand All @@ -1521,23 +1526,12 @@ _PyGC_GetObjects(PyInterpreterState *interp, int generation)
int err = gc_visit_heaps(interp, &visit_get_objects, &args.base);
_PyEval_StartTheWorld(interp);

PyObject *list = list_from_object_stack(&args.objects);
if (err < 0) {
PyErr_NoMemory();
goto error;
Py_CLEAR(list);
}

PyObject *op;
while ((op = _PyObjectStack_Pop(&args.objects)) != NULL) {
if (op != result && PyList_Append(result, op) < 0) {
goto error;
}
}
return result;

error:
Py_DECREF(result);
_PyObjectStack_Clear(&args.objects);
return NULL;
return list;
}

static bool
Expand Down

0 comments on commit b6b9b7a

Please sign in to comment.