Conversation
|
All commit authors signed the Contributor License Agreement. |
markshannon
added
needs backport to 3.13
|
Looks good to me, but I'd appreciate a review from @gaogaotiantian before merging |
|
@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! |
| } | ||
| if (should_de_instrument) { | ||
| MODIFY_BYTECODE(code, de_instrument_line, monitoring, offset); | ||
| /* Restore all thread-local bytecodes before updating the shared |
There was a problem hiding this comment.
The comment here is actually explaining what the line above is doing right? I think we should move this comment before the line above, or rephase it.
| code = func.__code__ | ||
| expected = [instr.offset for instr in dis.get_instructions(func) | ||
| if instr.opname != "RESUME"] | ||
| records = [[[], []] for _ in range(2)] |
There was a problem hiding this comment.
Took me forever to understand the structure. Let's explain a bit here - why the nested structures.
| def check_disable_line_keeps_instruction_events(self, local, line_tool): | ||
| def func(x): | ||
| a = x + 1 | ||
| b = ( |
There was a problem hiding this comment.
Is it necessary for this function to be like this? If we need it to be a multi-line statement, we better talk about it.
|
|
||
| def test_disable_line_keeps_instruction_events(self): | ||
| for local in (False, True): | ||
| for line_tool in (TEST_TOOL, TEST_TOOL2): |
There was a problem hiding this comment.
I think it's clearer to have line_tool and instr_tool listed here and explain that you want to test for cases where line_tool is the tool same as/different than instr_tool. Having the instr_tool hard-coded in the function makes it a bit more difficult to understand.
…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.
|
@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 Monitoring tests pass on standard and free-threaded debug builds (99 and 113 tests, respectively). Ready for another look when you have time. |
3 participants
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 forINSTRUMENTED_INSTRUCTIONin 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
596d923with this patch:test_free_threading.test_monitoringpassed (1,613 test cases; expected skips).python_d.exe -m test -R 3:3 test_monitoringpassed with no reference leaks.patchcheck, Ruff, NEWS reStructuredText lint, andgit diff --checkpassed.The complete CPython test suite and non-Windows builds were not run locally.