Add trigger_exception_handler() for post-mortem debugging of caught exceptions#1996
Add trigger_exception_handler() for post-mortem debugging of caught exceptions#1996nshepperd wants to merge 9 commits into
Conversation
|
See #1530. |
|
Thanks for the submission but I think this would need some sort of automated test before we'd accept it. And maybe a section in the readme.md explaining how to use it. |
8cd1d7b to
b286f0c
Compare
|
Actually I was wondering how to setup tests for this. I figured it out today. Hooray! |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I'm confused about something. pdb.post_mortem attaches the debugger to the process and starts debugging it. This won't. You have to be already attached, right? What would be the point of this then? Is it just so you don't have to break on exceptions in the debugger, but instead control when the break happens? |
|
I think ideally this would actually start VS code and open the debugger. Which might be doable, but would probably have to be configurable somehow. |
|
Yes, the point is so that you can eg. make a context manager or function decorator to debug exceptions that bubble past it instead of setting bespoke breakpoint, which isn't really directly possible for this. I find something like this indispensable for debugging things like api servers or unit tests (pytest) that have exception handlers that eat exceptions. With this you can just wrap your route handler or set an exception handler with conftest.py and launch the program under the debugger and everything just works. I think previously debugpy had the 'user uncaught exceptions' breakpoint added for similar use cases, but that's much less specific and in some cases has too many false positives to be useful. You're right this is a bit different from pdb.post_mortem() in that that actually starts the debugger. I'm not sure about starting vscode though, heh. Maybe it's possible via some sort of vscode:// url? What we could do is at least run debugpy.listen and wait for the debugger to attach? I'm not sure i would actually use that though, usually if i'd want to debug something with vscode I'm already debugging it (eg. I press the 'debug unit test' button) and the issue is just getting the breakpoint. |
|
Either way each use cases should work, i think. If you wanted to launch vscode and attach the debugger you can use existing debugpy functions for that and then call post_mortem. Or if we include that in the function you can check if debugpy.is_client_connected() before calling it if you don't want that. |
If this is the intended use case, maybe the name is off then. It's not post_mortem, it's more like 'force_exception_handler' or something like that. I feel like 'post_mortem' is intended to be like pdb means it. It starts a debugger even when it's not already attached. |
|
Your unit tests also seem to be failing. Not sure if you can see the errors or not |
|
I see the test failures. Cython issue. I forgot to regenerate the cython files. Confusing because it worked locally. |
|
|
…ht flag which says whether to apply uncaught exception breakpoint filters, defaulting to True.
…ost_mortem does. Also, add an explanation in the README.md
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… yet. If the calling thread had never hit a monitoring event that created its ThreadInfo, suspend_current_thread_tracing() was a no-op, and the first monitoring event during the manual do_wait_suspend would create a ThreadInfo with tracing enabled and re-enter do_wait_suspend, replacing the tracked exception frames list (making exceptionInfo report 'type unknown'). Create the ThreadInfo eagerly so tracing is really disabled. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5b9dde5 to
92434f4
Compare
|
@nshepperd please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
LGTM. I'll get somebody else to take a look. |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
|
🔒 Automated review in progress — @heejaechang is auto-reviewing this PR. |
heejaechang
left a comment
There was a problem hiding this comment.
Approved via Review Center.
| if PYDEVD_USE_SYS_MONITORING: | ||
| if saved_sys_monitoring_trace: | ||
| pydevd_sys_monitoring.resume_current_thread_tracing() | ||
| additional_info.is_tracing -= 1 |
There was a problem hiding this comment.
📍 src/debugpy/_vendored/pydevd/pydevd.py:2455
The Skeptic verified (reproduced with a snippet: tracing restored? False) that suspend_current_thread_tracing(), set_additional_thread_info, and is_tracing += 1 all run outside the try. If anything raises in that window — most realistically a KeyboardInterrupt during a live debug session, or a failure in set_additional_thread_info — the finally calling resume_current_thread_tracing() never runs, leaving thread_info.trace = False permanently and silently killing breakpoints on that thread for the rest of the session. Move the suspend + is_tracing += 1 setup inside the try so the finally always restores state.
| # explicitly disabled here, the first monitoring event on this thread | ||
| # would create it with tracing enabled. | ||
| thread_info = _get_thread_info(True, 1) | ||
| if thread_info is None: |
There was a problem hiding this comment.
📍 src/debugpy/_vendored/pydevd/_pydevd_sys_monitoring/_pydevd_sys_monitoring.py:1875
When the except branch freshly creates a ThreadInfo via _get_thread_info(True, 1) (tracing enabled), it still reports previous_state = True. The caller's finally then sees saved == True and re-enables tracing on a thread that was not previously traced, contradicting "restore previous state" and potentially causing unexpected breakpoint stops on background threads. When the ThreadInfo is created here rather than found, return False so the caller skips the resume. Mirror the same fix in the .pyx (and regenerate the .c).
| } | ||
| ) | ||
|
|
||
| session.request_continue() |
There was a problem hiding this comment.
📍 tests/debugpy/test_trigger_exception_handler.py:1
The PR's core justification — suspending sys.monitoring to avoid reentrancy — is untested. None of the 5 tests set a breakpoint condition that re-enters monitoring during the stop, and none assert that breakpoints still fire on the same thread after request_continue(). If suspend_current_thread_tracing() were a no-op (or the resume-leak bugs triggered), every test would still pass. Add a test that hits a real breakpoint on the same thread after trigger_exception_handler returns, proving tracing was correctly restored.
| raise | ||
| """ | ||
|
|
||
|
|
There was a problem hiding this comment.
📍 src/debugpy/public_api.py:215
The as_uncaught parameter is never documented in the public docstring or README, despite changing a non-obvious, inverted-feeling contract: True respects the uncaught-exception breakpoint filters (may not stop), while False forces post-mortem regardless. Document both as_uncaught semantics and note the as_uncaught=False + library-only-traceback case.
| frames.append(frame) | ||
| tb = tb.tb_next | ||
|
|
||
| if user_frame is None: |
There was a problem hiding this comment.
📍 src/debugpy/_vendored/pydevd/pydevd.py:2430
The as_uncaught=False path silently no-ops when the traceback contains only library frames (user_frame stays None → debug log + return). The README/docstring imply as_uncaught=False always enters post-mortem, so a caller wrapping a library-only exception gets a surprising silent non-stop. At minimum document this; consider surfacing it more loudly than a debug-level log.
|
GitHub cannot anchor PR review comments to unchanged lines in the diff. Falling back to a general PR comment for src/debugpy/server/api.py:L396. 📍 src/debugpy/server/api.py:366 |
| try: | ||
| if as_uncaught: | ||
| self.stop_on_unhandled_exception(self, thread, additional_info, excinfo) | ||
| else: |
There was a problem hiding this comment.
📍 src/debugpy/_vendored/pydevd/pydevd.py:2466
self.stop_on_unhandled_exception(self, thread, additional_info, excinfo) is correct — line 765 binds the module-level function as an instance attribute, so the explicit self is the intended py_db first arg. It is NOT a bug, but it reads like a double-self mistake and is a maintenance trap a future reader may "fix" and break. Add a one-line comment (e.g. # instance attr holds a module-level fn; first arg is py_db).
|
|
||
|
|
||
| exc_info = session.request("exceptionInfo", {"threadId": occ.body['threadId']}) | ||
| assert exc_info == some.dict.containing( |
There was a problem hiding this comment.
📍 tests/debugpy/test_trigger_exception_handler.py:152
The stray print("About to call trigger_exception_handler with excinfo") in test_trigger_exception_handler_excinfo is leftover debugging. Remove it.
|
Overall this is a solid, well-tested addition. The main theme worth addressing before/after merge is robustness of the sys.monitoring tracing state restoration: the suspend + is_tracing setup happens outside the try/finally, the 'previous state' value can be misleading for freshly-created ThreadInfo, and the core reentrancy-suspension behavior is not directly exercised by a test. Also a couple of documentation gaps around as_uncaught. None are blocking. |
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
|
Okay sorry, I had our automated tools run over it. It found some things to look at if you can. Thanks for the work :) |
|
You also need to do this before we can accept it: |
Adds a convenient trigger_exception_handler() function (previously named post_mortem(); renamed per review discussion) that can be called to immediately trigger the graphical postmortem debugger given any exc_info structure. We need to suspend sys.monitoring callbacks while doing this, because the built in reentrancy protection doesn't apply when pydevd suspends directly from normal code instead of a monitoring callback. Functions were added in _pydevd_sys_monitoring to do that.