Updates to sys.monitoring to get debugpy tests to pass

[rchiodo]

I submitted these changes (and plus all of the other sys.monitoring changes) to debugpy here:
microsoft/debugpy#1680

These changes get all of the tests in debugpy to pass with 3.12.

[rchiodo]

I changed the logic for get_unhandled_frame to just check if the passed in frame was the last user frame. When using Cython the old logic wasn't working because getFrame wasn't returning any of the pydevd frames and the logic was off.

[fabioz]

Humm that's odd, can you reproduce that with plain pydevd or just when debugpy is integrated?

It'd be nice to have a test for this in the pydevd repository to know what's actually happening to trigger the issue you're having (I've just made the main branch green for pydevd and tests should've picked issues in case this was wrong and in case they didn't, it'd be nice to have a new test to track the case).

[rchiodo]

The tests that were failing were these:

https://github.com/microsoft/debugpy/blob/25955a05d87a6e3d7694f7479c1c17ded0f9af90/tests/debugpy/test_exception.py#L160

The unhandled frame logic (when Cython was active) would fail to catch that something was unhandled.

[rchiodo]

Sorry still need to do this too. Haven't gotten to debugging this with the old code yet.

[rchiodo]

Going back to the old way, all of these tests fail in debugpy:

FAILED tests/debugpy/test_exception.py::test_systemexit[0-zero-uncaught-raised-attach_connect(cli)-program]
... (38 in total)

I'm looking into how to create a similar test in PyDev.Debugger.

[rchiodo]

Okay in debugpy these attach tests throw more unhandled exceptions, maybe because jmc isn't being applied?

I can see a whole bunch more exceptions:

0.00s - Ignore exception <class 'AttributeError'> in library C:\Users\rchiodo\AppData\Local\anaconda3\envs\py312_64\Lib\pathlib.py -- (__str__)
0.00s - Ignore exception <class 'AttributeError'> in library C:\Users\rchiodo\AppData\Local\anaconda3\envs\py312_64\Lib\pathlib.py -- (drive)
0.00s - Ignore exception <class 'FileNotFoundError'> in library C:\Users\rchiodo\AppData\Local\anaconda3\envs\py312_64\Lib\pathlib.py -- (open)
0.00s - Ignore exception <class 'FileNotFoundError'> in library C:\Users\rchiodo\AppData\Local\anaconda3\envs\py312_64\Lib\pathlib.py -- (open)
0.00s - Ignore exception <class 'FileNotFoundError'> in library C:\Users\rchiodo\AppData\Local\anaconda3\envs\py312_64\Lib\pathlib.py -- (read_text)
0.00s - Ignore exception <class 'FileNotFoundError'> in library C:\Users\rchiodo\AppData\Local\anaconda3\envs\py312_64\Lib\pathlib.py -- (read_text)
0.00s - File not traced (not in project): C:\Users\rchiodo\AppData\Local\anaconda3\envs\py312_64\Lib\site-packages\importlib_metadata\__init__.py
0.00s - Ignore exception <class 'FileNotFoundError'> in library C:\Users\rchiodo\AppData\Local\anaconda3\envs\py312_64\Lib\site-packages\importlib_metadata\__init__.py -- (read_text)
... // There's a bunch more

This results in this code here getting hit earlier in _get_unhandled_exception_frame

        if f_unhandled is not None:
            _thread_local_info.f_unhandled = f_unhandled
            return _thread_local_info.f_unhandled

But there's nothing that ever unsets this f_unhandled for the current thread. So it stays there.

This prevents future exceptions from being marked as unhandled because this exception frame is reused.

Removing this caching fixes the problem.

Trying to figure out a way to reproduce this in a test. Maybe have the test throw more exceptions in a different frame before the sys.exit.

[fabioz]

I think that the problem may be that it's just caching one when possibly other entries arise?

The logic here is that the cache is caching the first frame where things should be considered unhandled

So, in a stack such as

runpy.run
user code

it should mark runpy.run as the top thing, but there are cases when multiple top frames could be considered

i.e.:

threading.bootstrap1
threading.bootstrap2
user code

in this case all of those could be considered

So, the logic could be changed to keep on going up the stack and instead of keeping just 1 keep all that match to see if it works (so, instead of _get_unhandled_exception_frame it'd be _get_unhandled_exception_frames).

If that doesn't work, please just comment the cache out instead of doing the refactor and then this can be checked after the PR is merged.

[rchiodo]

Why is the cache needed in the first place? It seems like everytime an unwind event occurred it should be reset as it could be for an entirely different exception. At the very least, shouldn't the cache also keep track of the owning exception?

[rchiodo]

I updated the cache to clear itself it the cached frame isn't in the callstack for the new frame (which I believe should mean that the old frame was from a different exception).

[fabioz]

Well, the cache is a performance improvement thing (so, for correctness we could remove it for now, but for performance it may be good to add it back in the future if possible).

A note: in the refactoring you called it _is_user_frame, but that's not really correct, what it's trying to discover here is handling for unhandled exceptions and it's trying to determine if this is the frame that when reached the exception has to be considered unhandled (meaning it's the last frame before actually being raised as unhandled) -- the thing is that (in theory) this shouldn't really change for the same thread after it's computed once (apart from the corner case you found where apparently this is being computed too soon).

[fabioz]

As a note, especially on cases where subtle things were changed in the monitoring, the lack of tests along in the PR which check the actual use-case is the major blocker for the PR integration... Do you think you can add those?

As a note, I took some time to make the main branch green (some things were failyng because of updates out of the debugger), so, you can add more tests and verify that your changes don't break the current test suite (pydevd has a much higher test coverage on its internals than debugpy, so, I'm really interested on what tests slipped from the coverage that still did make it up to debugpy).

[fabioz]

Humm that's odd, can you reproduce that with plain pydevd or just when debugpy is integrated?

It'd be nice to have a test for this in the pydevd repository to know what's actually happening to trigger the issue you're having (I've just made the main branch green for pydevd and tests should've picked issues in case this was wrong and in case they didn't, it'd be nice to have a new test to track the case).

[fabioz]

Just merged. Thank you for the PR!

[rchiodo]

Thanks a lot for the feedback. I'll be submitting the changes for 3.13 next. Those were much smaller though :)