Repository navigation
sys.monitoring: local events still sent after free_tool_id #111963
Description
Bug report
Bug description:
I use set_local_events with a tool id. Later I free the tool id, but then still get those events. Shouldn't freeing the tool id stop all events?
import sys
print(sys.version)
sysmon = sys.monitoring
events = sysmon.events
myid = 1
def simple_function():
print("Inside simple function")
def sysmon_py_start(code, instruction_offset):
print(f"sysmon_py_start({code=}, {instruction_offset=})")
sysmon.set_local_events(myid, code, events.LINE)
def sysmon_line(code, line_number):
print(f"sysmon_line({code=}, {line_number=})")
print("Registering")
sysmon.use_tool_id(myid, "try669.py")
sysmon.set_events(myid, events.PY_START)
sysmon.register_callback(myid, events.PY_START, sysmon_py_start)
sysmon.register_callback(myid, events.LINE, sysmon_line)
simple_function()
print("Freeing")
sysmon.set_events(myid, 0)
sysmon.free_tool_id(myid)
simple_function()When I run this, I see:
3.12.0 (main, Oct 2 2023, 13:16:02) [Clang 14.0.3 (clang-1403.0.22.14.1)]
Registering
sysmon_py_start(code=<code object simple_function at 0x10ead1530, file "/private/tmp/try669.py", line 8>, instruction_offset=0)
sysmon_line(code=<code object simple_function at 0x10ead1530, file "/private/tmp/try669.py", line 8>, line_number=9)
Inside simple function
Freeing
sysmon_line(code=<code object simple_function at 0x10ead1530, file "/private/tmp/try669.py", line 8>, line_number=9)
Inside simple function
Shouldn't freeing the tool id stop sysmon_line from being called again?
The same behavior happens with 3.13.0a1.
cc @markshannon
CPython versions tested on:
3.12, 3.13
Operating systems tested on:
macOS
Activity
wip: prevent ghost local events
brandtbucher commentedon Nov 10, 2023
Agreed, events should be unset and callbacks should be unregistered when
free_tool_idis called.gaogaotiantian commentedon Nov 10, 2023
For now I don't think whether the tool_id is used is checked at all in the instrumentation process. It was only checked on higher level functions like
get_events().It's NOT against the documentation, but I also think it might be better to free all the hooks once the tool_id is freed.
Actually, I'd propose another API
sys.monitoring.clear_tool_id()to just clear all the hooks while keeping the tool_id.However, I believe the reason this is the way it is now, is because instrumentation is lazy-loaded. We don't track all the instrumented code objects and de-instrument them immediately once an event is unset.
Consider the following scenario:
As the instrumentation won't even trigger before
f()executes, whenf()executes, it'll thinktool_idis used - but it's actually already freed once.In order to achieve that, we may need to put a version number on all events, and on tool_id. That solution might not need a potentially growing piece of memory if the user just toggles tool_id for fun.
Just saying - it's not a trivial issue which we can resolve by adding an if statement somewhere.
brandtbucher commentedon Nov 10, 2023
Interesting. So it seems fairly straightforward to unregister callbacks, and clear global events, but not to clear local events.
In your example, though, I'd definitely expect neither tool to see the line events on
f. I guess I'd be +1 on versioning the set of local events per code object to make that happen.gaogaotiantian commentedon Nov 10, 2023
Actually, global events are lazily instrumented as well. So no that won't work either. Unregister callbacks are easy but that would be even worse if the events are not disabled.
gaogaotiantian commentedon Nov 10, 2023
For the version, it would be one version per event (local + global) per code object, plus a version for the tool id. Basically we need to version everything.
brandtbucher commentedon Nov 10, 2023
Hm, okay. This is definitely harder than it seems.
gaogaotiantian commentedon Nov 10, 2023
Yeah I guess that's why Mark did not do it to begin with. None of the instrumentation commands take effects immediately so the instrumentation need to somehow depend on the current state only.
We can disable the events if the tool_id is not currently used, that's easy, but that just hides the dirt under the carpet. Or we could say that it's the user's responsibility to clean everything up before freeing the id - it's a low level API.
If we do want a clean mechanism, some level of restructure might be needed unless I'm missing something obvious.
wip: prevent ghost local events
nedbat commentedon Nov 11, 2023
I've adapted by holding a WeakSet of the code objects I've set events on, and using it to clear the events when I'm shutting down. This seems to work. At least the documentation could be clearer on this.
29 remaining items
wip: prevent ghost local events
wip: prevent ghost local events
wip: prevent ghost local events
feat(pep669): prevent ghost local events
fix: prevent ghost local events in sysmon
fix: prevent ghost local events in sysmon
fix: prevent ghost local events in sysmon
fix: prevent ghost local events in sysmon
fix: prevent ghost local events in sysmon
fix: prevent ghost local events in sysmon
pythonGH-111963: Clarify sys.monitoring.free_tool_id's limitations (p…
nedbat commentedon Jul 30, 2024
I think this issue can be closed.
pythonGH-111963: Clarify sys.monitoring.free_tool_id's limitations (p…
gaogaotiantian commentedon Oct 1, 2024
Hi @nedbat. We added a
clear_tool_idAPI in #124568 to clear all the callbacks and events (global and local) for atool_id. Callingfree_tool_idwill also call that function first so all the registered instrumentation will be reverted. Hope that makes you happy :)I'll close this issue now because it's really fixed.