FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-151646: Fix data race between gc.get_stats() and GC in free-threading builds by zangjiucheng · Pull Request #151766 · python/cpython · GitHub

/ cpython Public
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension .c  (2) .h  (1) .py  (1) .rst  (1) All 4 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
6 changes: 6 additions & 0 deletions Include/internal/pycore_interp_structs.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,12 @@ struct _gc_runtime_state {
/* a permanent generation which won't be collected */
struct gc_generation permanent_generation;
struct gc_stats *generation_stats;
#ifdef Py_GIL_DISABLED
/* Protects generation_stats between gc_get_stats_impl() (reader) and
gc_collect_main() (writer); both resolve the stats slot (buffer index)
under this lock so they agree on which slot to touch (gh-151646). */
PyMutex stats_mutex;
#endif
/* true if we are currently running the collector */
int collecting;
// The frame that started the current collection. It might be NULL even when
Expand Down
99 changes: 99 additions & 0 deletions Lib/test/test_free_threading/test_gc_get_stats_race.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
# Reproduction for the gc.get_stats() data race under free-threading.
#
# CPython issue gh-151646: in free-threading builds (``--disable-gil``) the
# concurrent collector and ``gc.get_stats()`` touch the same per-generation
# statistics struct (``gcstate->generation_stats``) without any synchronisation:
#
# * READER -- ``gc_get_stats_impl()`` in ``Modules/gcmodule.c`` copies the
# ``struct gc_generation_stats`` for each generation (``collections``,
# ``collected``, ``uncollectable``, ``candidates``, ``duration``) with a
# plain struct assignment and no lock.
#
# * WRITER -- at the end of every collection cycle ``gc_collect_main()`` in
# ``Python/gc_free_threading.c`` mutates that very struct in place
# (``stats->collections++``, ``stats->collected += m``, ...), again with no
# lock.
#
# When one thread is collecting while several others read the stats, the
# unsynchronised read/write to the same memory is a data race. The values are
# only statistics, so the race is benign at the Python level (no crash), which
# is exactly what lets this script double as a regression test: once the fix
# adds proper synchronisation, ThreadSanitizer should stay quiet while the
# script keeps exiting cleanly.
#
# Running this script under a free-threading CPython build compiled with
# ThreadSanitizer (``./configure --disable-gil --with-thread-sanitizer``) is
# expected to print a ``WARNING: ThreadSanitizer: data race`` report whose stack
# traces point at ``gc_get_stats_impl`` (gcmodule.c) and ``gc_collect_main``
# (gc_free_threading.c).
#
# Standalone usage:
#
# ./python Lib/test/test_free_threading/test_gc_get_stats_race.py
#
# or as part of the test suite (only meaningful under a TSAN build):
#
# ./python -m test test_free_threading.test_gc_get_stats_race

import gc
import threading
import unittest

from test.support import threading_helper


# One thread hammers gc.collect() (the writer side); enough reader threads run
# gc.get_stats() concurrently to make the race easy to observe. Readers run
# until the writer is done so the test duration is controlled by the collection
# count rather than by reader loop counts.
NUM_READERS = 4
ITERATIONS = 50


def _stress_get_stats_race(num_readers=NUM_READERS, iterations=ITERATIONS):
"""Race gc.collect() against gc.get_stats()."""

# Synchronise the start so the writer and readers overlap for as long as
# possible, maximising the chance of the read and write landing on the
# statistics struct at the same time.
done = threading.Event()

def collector():
try:
for _ in range(iterations):
# Writer: each full collection updates gcstate->generation_stats.
gc.collect()
finally:
done.set()

def reader():
while not done.is_set():
# Reader: copies the per-generation stats structs with no lock.
gc.get_stats()

threading_helper.run_concurrently([collector] + [reader] * num_readers)


@threading_helper.requires_working_threading()
class TestGCGetStatsRace(unittest.TestCase):
def test_get_stats_collect_race(self):
_stress_get_stats_race()

# The race is benign at the Python level: gc.get_stats() must still
# return well-formed data and the interpreter must not crash.
stats = gc.get_stats()
self.assertEqual(len(stats), 3)
for generation in stats:
self.assertIn("collections", generation)
self.assertIn("collected", generation)
self.assertIn("uncollectable", generation)


if __name__ == "__main__":
# Standalone reproduction: run the race and exit cleanly so the script can
# be reused as a regression check once the fix lands.
print(f"Racing 1 gc.collect() thread against "
f"{NUM_READERS} gc.get_stats() thread(s), {ITERATIONS} collections...")
_stress_get_stats_race()
print("Done (no Python-level crash). "
"Run under a free-threading + TSAN build to observe the data race.")
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix a data race in free-threading builds between :func:`gc.get_stats` and a
concurrent garbage collection cycle. Access to the per-generation statistics
is now serialized with a mutex so the reader observes a consistent snapshot.
6 changes: 6 additions & 0 deletions Modules/gcmodule.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -374,9 +374,15 @@ gc_get_stats_impl(PyObject *module)
/* To get consistent values despite allocations while constructing
the result list, we use a snapshot of the running stats. */
GCState *gcstate = get_gc_state();
#ifdef Py_GIL_DISABLED
PyMutex_Lock(&gcstate->stats_mutex);
#endif
stats[0] = gcstate->generation_stats->young.items[gcstate->generation_stats->young.index];
stats[1] = gcstate->generation_stats->old[0].items[gcstate->generation_stats->old[0].index];
stats[2] = gcstate->generation_stats->old[1].items[gcstate->generation_stats->old[1].index];
#ifdef Py_GIL_DISABLED
PyMutex_Unlock(&gcstate->stats_mutex);
#endif

PyObject *result = PyList_New(0);
if (result == NULL)
Expand Down
4 changes: 3 additions & 1 deletion Python/gc_free_threading.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -2281,7 +2281,8 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason)
}
}

/* Update stats */
/* Update stats. */
PyMutex_Lock(&gcstate->stats_mutex);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It seems the guard macro Py_GIL_DISABLED is missing here and below the unlock.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The whole gc_free_threading.c file is wrapped in #ifdef Py_GIL_DISABLED so I don't think that's needed.

struct gc_generation_stats *stats = get_stats(gcstate, generation);
stats->ts_start = start;
stats->ts_stop = stop;
Expand All @@ -2290,6 +2291,7 @@ gc_collect_main(PyThreadState *tstate, int generation, _PyGC_Reason reason)
stats->uncollectable += n;
stats->duration += duration;
stats->candidates += state.candidates;
PyMutex_Unlock(&gcstate->stats_mutex);

GC_STAT_ADD(generation, objects_collected, m);
#ifdef Py_STATS
Expand Down
Loading

Back | FazBrowse Home | New Git URL