Skip to content

Commit

Permalink
fix: enforce thread safety for collector.data
Browse files Browse the repository at this point in the history
Testing on nogil failed with:

```
2024-06-13T07:27:44.0416467Z     def _clear_data(self) -> None:
2024-06-13T07:27:44.0417153Z         """Clear out existing data, but stay ready for more collection."""
2024-06-13T07:27:44.0417837Z         # We used to use self.data.clear(), but that would remove filename
2024-06-13T07:27:44.0418583Z         # keys and data values that were still in use higher up the stack
2024-06-13T07:27:44.0419145Z         # when we are called as part of switch_context.
2024-06-13T07:27:44.0419639Z >       for d in self.data.values():
2024-06-13T07:27:44.0420219Z E       RuntimeError: dictionary changed size during iteration
2024-06-13T07:27:44.0420572Z
2024-06-13T07:27:44.0420753Z coverage/collector.py:258: RuntimeError
```
  • Loading branch information
nedbat committed Jun 13, 2024
1 parent af3172b commit 827ce55
Show file tree
Hide file tree
Showing 8 changed files with 80 additions and 11 deletions.
3 changes: 3 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ Unreleased
the problem. They are now changed to mention "branch coverage data" and
"statement coverage data."

- Started testing on 3.13 free-threading (nogil) builds of Python. I'm not
claiming full support yet.


.. scriv-start-here
Expand Down
20 changes: 18 additions & 2 deletions coverage/collector.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

from __future__ import annotations

import contextlib
import functools
import os
import sys
Expand Down Expand Up @@ -255,14 +256,17 @@ def _clear_data(self) -> None:
# We used to use self.data.clear(), but that would remove filename
# keys and data values that were still in use higher up the stack
# when we are called as part of switch_context.
for d in self.data.values():
d.clear()
with self.data_lock or contextlib.nullcontext():
for d in self.data.values():
d.clear()

for tracer in self.tracers:
tracer.reset_activity()

def reset(self) -> None:
"""Clear collected data, and prepare to collect more."""
self.data_lock = self.threading.Lock() if self.threading else None

# The trace data we are collecting.
self.data: TTraceData = {}

Expand Down Expand Up @@ -305,10 +309,22 @@ def reset(self) -> None:

self._clear_data()

def lock_data(self) -> None:
"""Lock self.data_lock, for use by the C tracer."""
if self.data_lock is not None:
self.data_lock.acquire()

def unlock_data(self) -> None:
"""Unlock self.data_lock, for use by the C tracer."""
if self.data_lock is not None:
self.data_lock.release()

def _start_tracer(self) -> TTraceFn | None:
"""Start a new Tracer object, and store it in self.tracers."""
tracer = self._trace_class(**self._core_kwargs)
tracer.data = self.data
tracer.lock_data = self.lock_data
tracer.unlock_data = self.unlock_data
tracer.trace_arcs = self.branch
tracer.should_trace = self.should_trace
tracer.should_trace_cache = self.should_trace_cache
Expand Down
42 changes: 37 additions & 5 deletions coverage/ctracer/tracer.c
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ CTracer_dealloc(CTracer *self)
Py_XDECREF(self->should_trace_cache);
Py_XDECREF(self->should_start_context);
Py_XDECREF(self->switch_context);
Py_XDECREF(self->lock_data);
Py_XDECREF(self->unlock_data);
Py_XDECREF(self->context);
Py_XDECREF(self->disable_plugin);

Expand Down Expand Up @@ -470,26 +472,39 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame)
}

if (tracename != Py_None) {
PyObject * file_data = PyDict_GetItem(self->data, tracename);
PyObject * file_data;
BOOL had_error = FALSE;
PyObject * res;

res = PyObject_CallFunctionObjArgs(self->lock_data, NULL);
if (res == NULL) {
goto error;
}

file_data = PyDict_GetItem(self->data, tracename);

if (file_data == NULL) {
if (PyErr_Occurred()) {
goto error;
had_error = TRUE;
goto unlock;
}
file_data = PySet_New(NULL);
if (file_data == NULL) {
goto error;
had_error = TRUE;
goto unlock;
}
ret2 = PyDict_SetItem(self->data, tracename, file_data);
if (ret2 < 0) {
goto error;
had_error = TRUE;
goto unlock;
}

/* If the disposition mentions a plugin, record that. */
if (file_tracer != Py_None) {
ret2 = PyDict_SetItem(self->file_tracers, tracename, plugin_name);
if (ret2 < 0) {
goto error;
had_error = TRUE;
goto unlock;
}
}
}
Expand All @@ -498,6 +513,17 @@ CTracer_handle_call(CTracer *self, PyFrameObject *frame)
Py_INCREF(file_data);
}

unlock:

res = PyObject_CallFunctionObjArgs(self->unlock_data, NULL);
if (res == NULL) {
goto error;
}

if (had_error) {
goto error;
}

Py_XDECREF(self->pcur_entry->file_data);
self->pcur_entry->file_data = file_data;
self->pcur_entry->file_tracer = file_tracer;
Expand Down Expand Up @@ -1039,6 +1065,12 @@ CTracer_members[] = {
{ "switch_context", T_OBJECT, offsetof(CTracer, switch_context), 0,
PyDoc_STR("Function for switching to a new context.") },

{ "lock_data", T_OBJECT, offsetof(CTracer, lock_data), 0,
PyDoc_STR("Function for locking access to self.data.") },

{ "unlock_data", T_OBJECT, offsetof(CTracer, unlock_data), 0,
PyDoc_STR("Function for unlocking access to self.data.") },

{ "disable_plugin", T_OBJECT, offsetof(CTracer, disable_plugin), 0,
PyDoc_STR("Function for disabling a plugin.") },

Expand Down
2 changes: 2 additions & 0 deletions coverage/ctracer/tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ typedef struct CTracer {
PyObject * trace_arcs;
PyObject * should_start_context;
PyObject * switch_context;
PyObject * lock_data;
PyObject * unlock_data;
PyObject * disable_plugin;

/* Has the tracer been started? */
Expand Down
10 changes: 8 additions & 2 deletions coverage/pytracer.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ def __init__(self) -> None:
self.should_trace_cache: dict[str, TFileDisposition | None]
self.should_start_context: Callable[[FrameType], str | None] | None = None
self.switch_context: Callable[[str | None], None] | None = None
self.lock_data: Callable[[], None]
self.unlock_data: Callable[[], None]
self.warn: TWarnFn

# The threading module to use, if any.
Expand Down Expand Up @@ -209,8 +211,12 @@ def _trace(
if disp.trace:
tracename = disp.source_filename
assert tracename is not None
if tracename not in self.data:
self.data[tracename] = set()
self.lock_data()
try:
if tracename not in self.data:
self.data[tracename] = set()
finally:
self.unlock_data()
self.cur_file_data = self.data[tracename]
else:
frame.f_trace_lines = False
Expand Down
10 changes: 8 additions & 2 deletions coverage/sysmon.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ def __init__(self, tool_id: int) -> None:
# Change tests/testenv.py:DYN_CONTEXTS when this is updated.
self.should_start_context: Callable[[FrameType], str | None] | None = None
self.switch_context: Callable[[str | None], None] | None = None
self.lock_data: Callable[[], None]
self.unlock_data: Callable[[], None]
# TODO: warn is unused.
self.warn: TWarnFn

Expand Down Expand Up @@ -317,8 +319,12 @@ def sysmon_py_start(self, code: CodeType, instruction_offset: int) -> MonitorRet
if tracing_code:
tracename = disp.source_filename
assert tracename is not None
if tracename not in self.data:
self.data[tracename] = set()
self.lock_data()
try:
if tracename not in self.data:
self.data[tracename] = set()
finally:
self.unlock_data()
file_data = self.data[tracename]
b2l = bytes_to_lines(code)
else:
Expand Down
2 changes: 2 additions & 0 deletions coverage/tracer.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ class CTracer(TracerCore):
should_trace: Any
should_trace_cache: Any
switch_context: Any
lock_data: Any
unlock_data: Any
trace_arcs: Any
warn: Any
def __init__(self) -> None: ...
Expand Down
2 changes: 2 additions & 0 deletions coverage/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ class TracerCore(Protocol):
should_trace_cache: Mapping[str, TFileDisposition | None]
should_start_context: Callable[[FrameType], str | None] | None
switch_context: Callable[[str | None], None] | None
lock_data: Callable[[], None]
unlock_data: Callable[[], None]
warn: TWarnFn

def __init__(self) -> None:
Expand Down

0 comments on commit 827ce55

Please sign in to comment.