sys.monitoring: local events still sent after free_tool_id

[nedbat]

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

[brandtbucher]

Agreed, events should be unset and callbacks should be unregistered when free_tool_id is called.

[gaogaotiantian]

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:

def f():
    pass
sys.monitoring.use_tool_id(1, "tool1")
sys.set_local_events(1, f.__code__, LINE)
sys.monitoring.free_tool_id(1, "tool1")
sys.monitoring.use_tool_id(1, "tool2")
# What would happen here? Does the event fire?

As the instrumentation won't even trigger before f() executes, when f() executes, it'll think tool_id is 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]

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]

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]

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]

Hm, okay. This is definitely harder than it seems.

[gaogaotiantian]

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.

[nedbat]

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.

[nedbat]

But further experimentation drew me into the question of equality of code objects. It looks like the code_richcompare function looks at the bytecodes themselves. Doesn't the specializing interpreter change the bytecodes? Apart from the WeakSet of code objects, I'm using a dict with code object keys, and seeing some mysterious behavior where code objects are added and then can't be found. Could this be a problem?