Skip to content

gh-158121: Preserve instruction events when line monitoring is disabled - #158456

Merged
gaogaotiantian merged 4 commits into
python:mainfrom
limadog9:gh-158121-monitoring-disable
Oct 19, 2026
Merged

gh-158121: Preserve instruction events when line monitoring is disabled#158456
gaogaotiantian merged 4 commits into
python:mainfrom
limadog9:gh-158121-monitoring-disable

Conversation

@limadog9

@limadog9 limadog9 commented Sep 29, 2026 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

Fixes #158121.

When the last LINE callback returns sys.monitoring.DISABLE, the line metadata is updated to the underlying opcode. Dispatching that saved opcode skips the INSTRUCTION event for the current instruction even though instruction monitoring remains enabled. Check for INSTRUMENTED_INSTRUCTION in the current bytecode before using the saved opcode, while retaining the existing fallback when callbacks change monitoring.

Also defer updating the shared line metadata until every thread-local bytecode copy has been restored. Otherwise, later copies lose their INSTRUCTION wrappers and keep missing events on subsequent calls in free-threaded builds.

Regression tests cover global/local events, the same/separate tools, repeated calls, restart_events(), and two existing worker-thread bytecode copies. The first regression fails on unmodified main in both builds; the threaded regression also fails with only the dispatch fix.

Validation on Windows x64, built from main at 596d923 with this patch:

  • Debug GIL build: 13 relevant test modules passed (1,599 test cases; expected skips).
  • Debug free-threaded build: the same modules plus test_free_threading.test_monitoring passed (1,613 test cases; expected skips).
  • Suites include monitoring, tracing, profiling, pdb/bdb, disassembly, frames, generators, coroutines, async generators, and threading.
  • python_d.exe -m test -R 3:3 test_monitoring passed with no reference leaks.
  • Additional callback reconfiguration and prewarmed worker-thread probes passed in both builds.
  • patchcheck, Ruff, NEWS reStructuredText lint, and git diff --check passed.

The complete CPython test suite and non-Windows builds were not run locally.

@python-cla-bot

python-cla-bot Bot commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app bedevere-app Bot mentioned this pull request Sep 29, 2026
Closed
@markshannon markshannon added needs backport to 3.13 only security fixes needs backport to 3.14 bugs and security fixes needs backport to 3.15 bugs and security fixes labels Sep 30, 2026
@markshannon

markshannon commented Sep 30, 2026

Copy link
Copy Markdown
Member

Looks good to me, but I'd appreciate a review from @gaogaotiantian before merging

@limadog9 ChatGPT Codex Connector

limadog9 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

@gaogaotiantian Mark mentioned he'd appreciate your review before merging. When you have a chance, would you mind taking a look? Happy to address anything you spot. Thanks!

gaogaotiantian
gaogaotiantian reviewed Oct 2, 2026
View reviewed changes
Comment thread Python/instrumentation.c Resolved
Comment thread Lib/test/test_free_threading/test_monitoring.py Resolved
Comment thread Lib/test/test_monitoring.py Resolved
Comment thread Lib/test/test_monitoring.py Resolved
@limadog9
pythongh-158121: Clarify monitoring regression tests and opcode resto…
f7e198c 
…ration

Address review feedback by documenting the per-thread call records and multiline assignment, parameterizing both monitoring tool IDs, and placing the restoration comment before MODIFY_BYTECODE.

Validation: Windows x64 Debug test_monitoring (99 tests); free-threaded Debug test_monitoring and test_free_threading.test_monitoring (113 tests); Ruff, patchcheck, and git diff --check.
@limadog9 ChatGPT Codex Connector

limadog9 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

@gaogaotiantian Thanks for the review! Addressed all four comments in f7e198c: moved the C comment before bytecode restoration, explained the nested thread/call records, documented the multiline assignment's backward line-event coverage, and made both line_tool and instr_tool explicit in the test cases.

Monitoring tests pass on standard and free-threaded debug builds (99 and 113 tests, respectively). Ready for another look when you have time.

@limadog9 ChatGPT Codex Connector

limadog9 commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

@gaogaotiantian Just following up on the review updates in f7e198c. Could you take another look when you have a chance? Thanks!

@limadog9 ChatGPT Codex Connector

limadog9 commented Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Hi @gaogaotiantian, just checking in on the review updates in f7e198c. Your four comments have been addressed, and CI is passing. When you have a chance, could you take another look and let me know if anything else is needed before merging? Thanks again for your time!

@gaogaotiantian gaogaotiantian removed the needs backport to 3.13 only security fixes label Oct 19, 2026
@gaogaotiantian
gaogaotiantian merged commit f0f76de into python:main Oct 19, 2026
60 checks passed
@miss-islington-app

miss-islington-app Bot commented Oct 19, 2026

Copy link
Copy Markdown

Thanks @limadog9 for the PR, and @gaogaotiantian for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15.
🐍🍒⛏🤖

@miss-islington-app

miss-islington-app Bot commented Oct 19, 2026

Copy link
Copy Markdown

Sorry, @limadog9 and @gaogaotiantian, I could not cleanly backport this to 3.14 due to a conflict.

Please backport manually with cherry_picker, see the devguide for more information.

cherry_picker f0f76de0c8521e51c2e5c1da6de23373ec39231c 3.14

@bedevere-app

bedevere-app Bot commented Oct 19, 2026

Copy link
Copy Markdown

GH-159733 is a backport of this pull request to the 3.15 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.15 bugs and security fixes label Oct 19, 2026
@gaogaotiantian

gaogaotiantian commented Oct 19, 2026

Copy link
Copy Markdown
Member

Sorry I've been super busy last 2 weeks. Thank you for fixing this!

@gaogaotiantian gaogaotiantian mentioned this pull request Oct 19, 2026
Merged
@bedevere-app

bedevere-app Bot commented Oct 19, 2026

Copy link
Copy Markdown

GH-159741 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Oct 19, 2026
gaogaotiantian pushed a commit that referenced this pull request Oct 19, 2026
@miss-islington @limadog9
[3.15] gh-158121: Preserve instruction events when line monitoring is…
64e7708 
… disabled (GH-158456) (#159733)

gh-158121: Preserve instruction events when line monitoring is disabled (GH-158456)
(cherry picked from commit f0f76de)

Co-authored-by: limadog9 <jakewallin42@gmail.com>
gaogaotiantian added a commit that referenced this pull request Oct 19, 2026
@gaogaotiantian @limadog9
[3.14] gh-158121: Preserve instruction events when line monitoring is… (
a436cd2 
#159741)

* [3.14] gh-158121: Preserve instruction events when line monitoring is disabled (GH-158456)
(cherry picked from commit f0f76de)

Co-authored-by: limadog9 <jakewallin42@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaogaotiantian gaogaotiantian gaogaotiantian approved these changes

Assignees

@gaogaotiantian gaogaotiantian

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

sys.monitoring: INSTRUCTION event skipped when another tool's LINE callback returns DISABLE at the same instruction

3 participants

@limadog9 @markshannon @gaogaotiantian