Rehabilitating #431 (1/2?): threading fix + patching infrastructure

[TTsangSC]

This PR is the first in a series where code changes in #431 (plus review feedback) are broken down into more review-friendly chunks.

Main changes (relative to main)

• Fixed handling of sys.monitoring callbacks: now the callbacks themselves are process-global (see bac55fe), and the set[LineProfiler] passed to the C++-level inner tracing callback is thread-local, allowing for behavior that is (1) to-spec and (2) consistent across backends
• Refactored kernprof to make heavier use of context managers for setup and teardown
• Updated line_profiler.LineStats to allow for subtraction of a "baseline" value, to be used later when we handle profiling data from forked processes
• New code:
  • line_profiler._threading_patches:
    New module for threading patches so that profiling continues seamlessly into child threads
  • line_profiler.cleanup, line_profiler.curated_profiling:
    New modules for context-based management of profiling-session setup and teardown

Changes since last round of #431 review

Code changes

• line_profiler._line_profiler.pyx:
  • Incorporated code from bac55fe (see Fable review of profile child process TTsangSC/line_profiler#5 by @Erotemic), fixing the previous assumption that sys.monitoring callbacks are thread-local (they aren't); but see the following for change on top thereof.
  • The callbacks passed to sys.monitoring.register_callback() are now instance methods on _SysMonitoringState instead of _LineProfilerManager, and _SysMonitoringState.active_instances are now thread-local, allowing for thread-local LineProfiler.enable_count to be handled consistently.
  • Updated _LineProfilerManager._handle_enable_event() and ._handle_disable_event() so that calls to _SysMonitoringState.register() and .deregister() are made in consideration of the process-global states, not just the thread-local .active_instances.
• line_profiler/_threading_patches.py:
  • Superseded make_thread_init_wrapper() with wrap_thread_start() to prevent issues with delayed start.
  • Instead of using make_syncing_wrapper() to directly wrap Thread._target, we now wrap Thread._bootstrap() and .run() so that the patch is effective on Thread subclasses with an overridden .run() method.
  • The thread is now applied regardless of ${LINE_PROFILER_CORE} used; the previous behavior where it seemed to be gratuitous when using sys.monitoring was only an artifact of how the callbacks were erroneously handled (see above).
  • As such, SHOULD_PATCH_THREADING is removed.
• line_profiler/c_trace_callbacks.c:
  Integrated changes from a5001ca (see Fable review of profile child process TTsangSC/line_profiler#5 by @Erotemic), fixing the leaking of frame-local legacy trace callbacks.
• line_profiler/line_profiler.py:
  • get_code_block():
    Now making use of the new clone_single_module() to create a private copy of inspect with the patched facilities, instead of monkey-patching the global module.
  • LineStats.from_files():
    Fixed error handling.
• line_profiler/line_profiler_utils.py::clone_single_module():
  New helper function for constructing a copy of a module.

Test changes

• line_profiler/*.py:
  Added/extended doctests in the aforementioned modules for coverage.

• tests/test_threading.py:
  New test module.

  • test_disable_on_one_thread_keeps_other_threads_profiling():
    Migrated and refactored from tests/test_cross_thread_profiling.py (bac55fe), making sure that disabling a profiler on a thread does not affect profiling on another thread by either the same profiler or a different one
  • Tests for various (edge) cases proposed in the review:
  • test_child_thread_profiling_toggle_by_count():
    Where the .enable_count has been altered between thread creation and starting.
  • test_child_thread_profiling_subclassed():
    Where a Thread subclass with an overridden .run() is used.
  • test_child_thread_profiling_separate_creation_and_consumption():
    Where a Thread object is instantiated and used on different threads.
  • test_child_thread_profiling_in_kernprof():
    End-to-end test with kernprof.

• tests/test_line_profiler.py:

  • test_load_stats_files_backward_compatiblity():
    Renamed from test_load_stats_files().
  • test_load_problematic_stats_file():
    New test proposed in the review for the protocols ('ignore', 'warn', or 'error') of handing missing, empty, corrupted, and incompatible stats files.

• tests/test_sys_trace.py::test_trace_wrappers_are_not_leaked():
  Migrated and refactored from tests/test_trace_callback_leaks.py (a5001ca), making sure that the "disabling wrappers" created in line_profiler._line_profiler are not leaked

Misc. fixes

• kernprof.py::_pre_profile():
  Fixed bug where sys.argv is replaced with a different list[str] object, thus preventing line_profiler.line_profiler_utils.restore from restoring its content.
• line_profiler/toml_config.py::ConfigSource.get_subconfig():
  Fixed bug where copy=True was not respected; also extended doctest for the case.
• tests/test_docs_conf.py:
  Fixed bug where a file object was created without being properly closed, resulting in noisy warnings as pytest exited.

TODO

In the following PRs:

• Incorporate the remaining changes from FEAT: extend profiling to child processes #431:
  • The entirety of line_profiler/_child_process_profiling
  • The hook _line_profiler_hooks.py
  • Config/CI changes in .github/workflows/tests.yml, setup.py, and line_profiler/rc/line_profiler.toml
  • The entirety of tests/test_child_procs
  • The behavior toggles and hooks to line_profiler._child_process_profiling in kernprof.py
• Review and either fix (where appropriate) or document the edge cases named in the review:
  • Race conditions between the Pool._result_handler and ._worker_handler threads.
  • Handling of the multiprocessing server processes:
    • Instead of setting up and tearing down in the .resource_tracker process, try to refactor the setup so that is not run there to begin with.
    • Defer setup (esp. the handling of eager pre-imports) of child processes served by the .forkserver to the children themselves instead of the server.
  • Failures in line_profiler components propagating to user-space and affecting the normal execution of the profiled code.
  • Interaction with kernprof --output-interval
• Add documentation for the new feature.

[codecov]

Codecov Report

❌ Patch coverage is 95.05618% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.48%. Comparing base (b5ca752) to head (71efcea).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
line_profiler/line_profiler.py	90.41%	4 Missing and 3 partials ⚠️
line_profiler/cleanup.py	96.24%	2 Missing and 3 partials ⚠️
line_profiler/curated_profiling.py	94.56%	2 Missing and 3 partials ⚠️
line_profiler/line_profiler_utils.py	96.87%	2 Missing and 1 partial ⚠️
line_profiler/_threading_patches.py	95.74%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #442      +/-   ##
==========================================
+ Coverage   84.95%   86.48%   +1.53%     
==========================================
  Files          21       24       +3     
  Lines        2412     2841     +429     
  Branches      376      429      +53     
==========================================
+ Hits         2049     2457     +408     
- Misses        263      273      +10     
- Partials      100      111      +11     

Files with missing lines	Coverage Δ
line_profiler/toml_config.py	92.64% <100.00%> (+0.16%)	⬆️
line_profiler/_threading_patches.py	95.74% <95.74%> (ø)
line_profiler/line_profiler_utils.py	96.44% <96.87%> (+0.39%)	⬆️
line_profiler/cleanup.py	96.24% <96.24%> (ø)
line_profiler/curated_profiling.py	94.56% <94.56%> (ø)
line_profiler/line_profiler.py	95.00% <90.41%> (-0.78%)	⬇️

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 984ad72...71efcea. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TTsangSC]

Got struck by works-on-my-machine-itis again, dunno why the threading stuff is not working on 3.10 and 3.11 in CI. More curiously is how the build jobs passed but the test jobs failed. May have to do with venv/uv weirdness or the lack thereof...

EDIT of course it's coverage. Let's see what exactly went wrong...

EDIT 2 fixed

[Erotemic]

🧑‍🦱: I went through and did a non-LLM read through to highlight some improvements that could be made wrt to making it easier for humans to read and understand the code. I think having module level docstrings more clearly introduce the mental model for the underlying code would make a big difference.

I then did a GPT 5.6 review, and discussed with it, and pasted relevant points near the code they belong to. I agree with it that we want to be thinking about state management transactionally. It also caught bugs I would have had a very difficult time catching myself.

🤖 The GPT 5.6 summary is:

Overall assessment

The central threading work has improved substantially. In particular, the patch now:

• captures profiler state at Thread.start() rather than at thread construction;
• handles a Thread subclass that overrides run();
• handles thread construction and starting on different threads;
• fixes the process-global lifetime of the sys.monitoring registration;
• contains a targeted regression for one thread disabling while another remains active;
• incorporates the C-level frame-trace reference leak fix;
• has broad green CI on Python 3.10–3.14, Linux/macOS/Windows, including ARM.

So the important problems with the earlier threading implementation are genuinely addressed.

Things I checked that look good

The new _SysMonitoringState structure is a substantial improvement over the previous version. Keeping registration/callback ownership process-global while selecting active_instances from a thread-specific set matches the actual sys.monitoring model much better. The new cross-thread regression with both one shared profiler and two separate profilers is particularly useful.

The C reference-management changes in c_trace_callbacks.c also look right: the newly created wrapper owns its temporary reference, assignment to f_trace creates/maintains the frame's reference, and the temporary is then decref'd. The weakref regression test is an appropriate test for the original leak.

The Thread.start() timing change and wrapping of run() instead of _target directly address the earlier delayed-start, overridden-run, and separate creator/starter cases. Those are no longer concerns I would repeat from the previous review.

The LineStats.from_files() error-handling additions look coherent, and the tests cover empty, nonexistent, corrupt, and incompatible inputs under the ignore/warn/error modes. I did not find a merge-blocking problem there.

Likewise, the LineStats baseline subtraction is reasonable groundwork for the later fork aggregation work. I would expect its full semantics to be exercised when the child-process half lands rather than expanding this PR further.

Suggested priority

I would fix these before approval:

1. descriptor-safe Cleanup.patch() restoration / Thread.run corruption;
2. exact restoration of GlobalProfiler state;
3. actual nested-context reentrancy;
4. define and correctly implement ownership/composition for the process-global threading patch.

I would also add the explicit threading.settrace() regression.

Transactional failure containment can reasonably remain partially deferred given the PR's stated split, but CuratedProfilerContext itself should ideally establish the invariant now that if installation fails, everything it already changed is reverted.

After those fixes, I think this first half of the #431 rehabilitation is structurally much closer to something I would want to build the child-process work on.

[Erotemic]

I don't think this module level docstring is sufficient for a new developer trying to reason how all the pieces in this repo fit together.

[Erotemic]

I would much rather define this default as a private named method somewhere than have a large lambda in a function signature.

[Erotemic]

Not sure how important this pattern is for your work, but whenever we can avoid passing functions as arguments, that makes the code MUCH easier to reason about. I get that its only for debug formatting, but these are the sort of highly dynamic things python lets you do that makes sense when you're writing code, but makes it harder for someone reading it. With LLMs it might not matter all that much, but its my preference.

[TTsangSC]

IIRC I did the formatting-function thing so that the cache object in #431 can format debug-log messages in a way that makes sense there (can be converted to and from log-entry objects) but not here.

But I probably (1) messed up the signature either way since it should probably have taken a str instead of a path, and (2) can pre-format the message in a wrapper/overridden method before calling this. Will think a bit harder about the refactoring.

[TTsangSC]

Never mind, it was supposed to take a Path because the method generates a new Path. But yeah as with .update_mapping() I will extract the argument into a private method.

[Erotemic]

The type in the docstring doesn't match the type in the signature. I think given that we have embraced inline typing, it's no longer necessary to even specify the type in the doc-string. However, I do think there should be a colon after the variable name. So the pattern would be:

<varname> [(<type>)]: <description>, where brackets around (type) indicates it is optional. (Not sure if this is accepted by sphinx, or if there is an existing parser for google style docstrings without typing, but it probably wouldn't be a huge lift to patch it in).

[Erotemic]

I'd suggest naming this CleanupStack to make it more clear what it does where it is used. It also looks like it doesn't always need to be used as a context manager, so maybe make the mental model for how someone needs to think about this object more clear in its docstring.

[Erotemic]

I'm looking at this more, and the semantics read weird. Say you have:

cleanup = Cleanup()

and then you call cleanup.patch, you are mutating an object, but the idea is that its done in a revertible way. But the name "Cleanup" doesn't really communicate that not only is this able to revert or clean up temporary state, it is also able to create that state and register it. I think something like RegisteredReversibleState is a bit too verbose. I discussed the issue with GPT and it suggested several names:

Details

Names that communicate both halves better:

• ReversibleState — probably my favorite. It says the object manages state that it is allowed to change, and those changes are reversible.
• StateTransaction — very strong if you want database-like semantics: mutate within a transaction, then roll back/restore at the end. This may actually be the clearest conceptual model.
• MutationScope — clearly says “mutations happen here,” with scope implying bounded lifetime. It does not quite communicate automatic restoration by itself.
• ReversibleMutation / ReversibleMutations — extremely explicit, though slightly awkward as a class holding many operations.
• MutationRegistry — communicates recording mutations, but not restoration strongly enough.
• StatePatch / StatePatcher — makes state.patch(...) feel natural, but underplays arbitrary cleanup callbacks.
• ScopedState — good lifecycle meaning, weaker on active mutation.
• StateOverride / StateOverrides — good if most operations are temporary replacements; weaker for arbitrary cleanup.
• RevertibleState — same idea as ReversibleState; I'd prefer “reversible” stylistically.

Of which I kind of like StateTransaction. That seems conceptually like what you're building here, unless I'm misunderstanding.

[TTsangSC]

Thinking a little bit harder about this I'm not so sure about the new suggested semantics. The Cleanup class is meant to be a holder of stacks of callbacks, which are executed in (reversed) order at "the end", whether it be an explicit call to .cleanup() or a context exit... so CleanupStack is indeed correct.

The helper methods .patch(), .update_mapping(), and .make_tempfile() implemented on top of that can be considered to be effectively logging transactions (changes + how to reverse them), but that seem to be one level of abstraction above callbacks. Still, maybe it makes sense to redesign the whole thing around transactions...

[Erotemic]

This one is from GPT 5.6: 🤖

1. Cleanup.patch() corrupts Thread.run after a thread exits

This is the clearest correctness bug I found.

wrap_thread_bootstrap() does:

with Cleanup() as cleanup:
    run = make_syncing_wrapper(self.run, prof, enable_count)
    cleanup.patch(self, 'run', run)
    vanilla_impl(self, *args, **kwargs)

But Cleanup.patch() defaults to static=True and saves the old value with inspect.getattr_static().

For a normal Thread instance, run is inherited from its class. Therefore:

inspect.getattr_static(thread, 'run')

returns the unbound function descriptor, not a bound method and not an instance attribute.

Cleanup later does essentially:

setattr(thread, 'run', old_function)

instead of deleting the temporary instance attribute. The result is that after the thread finishes, the thread object has a new run entry in its instance dictionary containing an unbound function.

I reproduced the underlying behavior on Python 3.13:

before-own-run False function
after-own-run True function
TypeError T.run() missing 1 required positional argument: 'self'

So profiling a thread permanently changes that thread object's method resolution, and calling thread.run() afterward fails.

This also exposes a flaw in the generic Cleanup.patch() abstraction: restoration needs to preserve where the attribute originally lived, not merely the value returned by getattr_static().

A robust implementation should distinguish at least:

• attribute originally present directly on the target object → restore that exact direct value;
• attribute originally inherited / absent from the target namespace → delattr() on cleanup, revealing the inherited attribute again.

For the threading code specifically, a regression test should assert after join() that an originally inherited run remains inherited:

assert 'run' not in vars(thread)

and that calling/binding it still works normally.

[Erotemic]

🤖 GPT 5.6 says:

2. CuratedProfilerContext leaves line_profiler.profile in an invalid state

CuratedProfilerContext.install() temporarily installs its profiler with:

self._global_install(self.prof)

and registers cleanup as:

self.add_cleanup(self._global_install, None)

But _global_install() delegates to GlobalProfiler._kernprof_overwrite(), whose implementation is:

def _kernprof_overwrite(self, profile):
    self._profile = profile
    self.enabled = True

Therefore teardown does:

_global_profiler._profile = None
_global_profiler.enabled = True

That is not a restoration. It is an internally inconsistent GlobalProfiler.

The next use of the decorator can reach:

if not self.enabled:
    return func
assert self._profile is not None

and fail the assertion.

This behavior existed as part of the old kernprof end-of-process cleanup, where it was largely hidden because the CLI was about to terminate. Moving it into a reusable context-management abstraction makes it observable in a long-lived process.

The context should snapshot and restore the previous _profile and enabled state exactly, preferably through an explicit temporary-override API on GlobalProfiler rather than by treating _kernprof_overwrite(None) as an uninstall operation.

A regression test should look roughly like:

before = (profile._profile, profile.enabled)

with CuratedProfilerContext(prof):
    ...

assert (profile._profile, profile.enabled) == before

It would also be useful to actually decorate/call a function afterward.

[Erotemic]

Also related from 🤖 GPT 5.6:

5. CuratedProfilerContext.install() still lacks transactional rollback

This is somewhat different because the PR explicitly lists broader failure-containment work as deferred, so I would distinguish it from the definite bugs above.

However, the new abstraction currently performs multiple global mutations:

upgrade profiler
replace global @profile
patch threading
possibly patch builtins
register teardown

without an install()-local rollback guard.

_manage_profiler.__enter__() does attempt cleanup if _prepare_exec_script() fails, but its try starts after:

self._ctx.install()

So an exception partway through install() itself can still leave process-global state partially modified.

Since CuratedProfilerContext is specifically becoming the authority for session setup/teardown, I would make the operation transactional now even if broader child-process failure containment stays in the next PR:

try:
    ...
except BaseException:
    self.cleanup(reason='failed profiler context installation')
    raise

There should be a fault-injection test that forces failure after at least one global patch and verifies complete restoration.

[Erotemic]

🤖 GPT 5.6 says:

3. The documented reentrancy of CuratedProfilerContext is broken

Its docstring says nested reentry of the same context is supported and that the nested entry is a no-op.

The implementation is:

def install(self):
    if self._installed:
        return
    ...
    self.patch(self, '_installed', True)

def __enter__(self):
    self.install()
    return self

def __exit__(self, *_, **__):
    self.uninstall()

Thus:

with ctx:
    with ctx:
        pass
    # ctx has already been uninstalled here

The inner __enter__() is a no-op, but the inner __exit__() is not a no-op. It removes all of the outer context's patches.

This should either have an acquisition depth, where only the outermost exit performs teardown, or reentrancy should be explicitly rejected and the documentation changed. Given that the base Cleanup already has stack concepts, depth-aware ownership seems preferable.

This deserves a direct nested-context test; the current doctests do not exercise it.

[TTsangSC]

Yeah I shouldn't have directly inherited from Cleanup as you mentioned earlier. That way we also don't have to be reentrant ;)

[Erotemic]

🤖 GPT 5.6 says:

4. The process-global threading patch silently belongs to whichever profiler installed it first

_threading_patches.apply() is:

if getattr(threading, _PATCHED_MARKER, False):
    return

start_wrapper = wrap_thread_start(prof, threading.Thread.start)
cleanup.patch(threading.Thread, 'start', start_wrapper)
cleanup.patch(threading, _PATCHED_MARKER, True)

The global Thread.start wrapper closes over exactly one prof.

If two distinct curated profiling contexts overlap:

1. context A patches Thread.start, capturing profiler A;
2. context B sees _PATCHED_MARKER and does nothing;
3. threads started while both are active only inherit profiler A;
4. if context A exits first, it restores Thread.start even though B remains active.

This is a global-resource ownership problem analogous to the sys.monitoring problem that this PR correctly fixed at the Cython layer.

I think the cleaner architecture is for the global thread hook not to belong to a particular LineProfiler at all. Install a single process-global hook which, at Thread.start(), snapshots the relevant active profiler/count state for that starting thread and arranges for those states to be activated in the child. Then separately manage installation ownership/reference counts.

If multiple concurrent curated sessions are intentionally unsupported, that constraint should instead be enforced explicitly. The current first-one-wins behavior is difficult to reason about.

[TTsangSC]

Right now the patch is session-bound and it's the session's profiler instance that uses it. But we can probably do better by dynamically fetching the active profiler instances on the starting thread and syncing all of them.

May have to think harder about whether we want to cater to the overlapping context use-case though. Worst case scenario, it isn't as modular as we'd want it to be, and we just fall back to having ~.curated_profiling be private. But I'll see if I can do a cleaner fix.

[TTsangSC]

On second thought syncing .enable_count on all LineProfiler instances (active on the starting thread) is probably not a good idea – we should probably only be doing that to profiler instances passed to the CuratedProfilingContext. Anyway, will think a bit more about having the multiple-profilers-per-context and multiple-contexts cases.

EDIT: most of the changes made by CuratedProfilerContext (as inherited from kernprof) already assume the existence of a "main profiler":

• It is interpolated into the builtins namespace with the name 'profile'.
• It is installed as the LineProfiler instance backing the public GlobalProfiler instance (line_profiler.profile).

So maybe it simply doesn't make sense for there to be more than one contexts and "main profilers" at any given time, which I think is probably okay as long as the limitation is well-documented. Meanwhile, I think ~._threading_patches can still be updated to accommodate multiple profiler instances.

[Erotemic]

🤖 GPT 5.6 says:

Important missing regression test: threading.settrace()

The commit fixing the CI/coverage interaction introduces the extra _bootstrap → run wrapping specifically because threading.settrace() installs the trace function inside _bootstrap_inner().

That code is marked # nocover, understandably because coverage itself participates in this mechanism. But I would still add an isolated subprocess regression that explicitly installs a threading.settrace() callback under the legacy backend and verifies both:

1. the foreign callback runs; and
2. line-profiler collects the expected statistics.

Otherwise the behavior that motivated wrap_thread_bootstrap() is largely being tested incidentally by running pytest under coverage rather than captured as an explicit contract.

[TTsangSC]

Yeah should've done that. Right now I do know that it works because that was what messed the first pipeline up (because coverage used threading.settrace()), but we should have an explicit test for it anyway.

[TTsangSC]

Thanks for all the comments! Will work on them when I'm back (am out in the city on an apartment hunt; wish me luck).