Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

userUnhandled should be supported via exceptionBreakpointFilters #275

Closed
karthiknadig opened this issue Jun 2, 2020 · 28 comments
Closed
Assignees

Comments

@karthiknadig
Copy link
Member

We should add userUnhandled to exceptionBreakpointFilters. This will need change in both debugpy side and pydevd side, to fully support #111

/cc @fabioz

@fabioz fabioz self-assigned this Jun 3, 2020
@fabioz
Copy link
Collaborator

fabioz commented Jun 3, 2020

I had actually added it in the debugpy side already (63c0fae#diff-f9ef1bd57538927eef0a8a0a2234e7d8R162), so, it should be already working on the VSCode side... I'll also update it in the pydevd side for completeness.

@fabioz
Copy link
Collaborator

fabioz commented Jun 3, 2020

p.s.: @karthiknadig @int19h I made the default true, do you think it should be false?

@karthiknadig
Copy link
Member Author

This should be false by default.

fabioz added a commit to fabioz/debugpy that referenced this issue Jun 3, 2020
fabioz added a commit to fabioz/debugpy that referenced this issue Jun 3, 2020
fabioz added a commit to fabioz/debugpy that referenced this issue Jun 3, 2020
@int19h int19h closed this as completed in f55bea7 Jun 4, 2020
@diginikkari
Copy link

How this can be enabled? There is no option in breakpoint list in vscode for userUnhandled, only for Raised and Ungaught. (vscode-python version 2020.5.86806)

@int19h
Copy link
Contributor

int19h commented Jun 11, 2020

@diginikkari This is intentional, as we're still working on some issues around this feature. The only way to enable it right now is to edit your local copy of debugpy. Stay tuned :)

@pwinston
Copy link

pwinston commented Jun 23, 2020

Is there an open issue tracking this @int19h, since this issue is closed?

Pytest does have a --pdbcls flag that allows you to "specify a custom debugger class" but I guess debugpy does not provide a compatible class?

I posted to stack overflow asking how to make vscode break with "unhandled" pytest exceptions, since I was so confused it cannot do that today:
https://stackoverflow.com/questions/62419998/how-can-i-get-pytest-to-not-catch-exceptions

It seems like if pytest just had a flag to not catch exceptions, it would work fine. But although it has 50-100 command line options it doesn't have one that does that, that I can see, which is surprising to me. I'm guessing they felt that pytest itself should never crash. But in the special case of debugging, it would be nice if it would crash, so we could debug the error which is being raised by our code.

@int19h
Copy link
Contributor

int19h commented Jun 24, 2020

@pwinston
We don't have any special support for pytest - it's debugged exactly the same as any other Python application.

The last known issue with user-unhandled exceptions was #281, which has since been fixed. We're still testing the feature and debating the UX, though. There are no tracking items for that, but it's a good idea. Since this is fully implemented on debugpy side, and the remaining work is entirely about integrating it properly into the Python extension for VSCode, I suggest filing an enhancement request on https://github.com/microsoft/vscode-python, and referencing this issue from there.

@fabioz
Copy link
Collaborator

fabioz commented Jun 24, 2020

@pwinston, as a note, you can manually enable the UX locally (until it's properly integrated) by doing the following:

Run a script with the contents below under the debugger:

import debugpy
import os.path
print(os.path.join(os.path.dirname(debugpy.__file__), 'adapter', 'clients.py'))

Then open the file printed and uncomment the line that has the contents #{"filter": "userUnhandled", "label": "User Uncaught Exceptions", "default": False}, (inside of def initialize_request(self, request)).

After that you should be able to select User Uncaught Exceptions in the breakpoints tab to enable this feature.

If you do that, please report if anything doesn't work as expected ;)

@pwinston
Copy link

pwinston commented Jun 24, 2020

Not sure it was necessary but I followed these instructions to install insider's python extension:
#59. Then I followed your instructions to modify clients.py and I saw the new User Uncaught Exceptions check box which was very exciting! Unfortunately when I checked only that box, it stopped at every handled exception, just as like Raised Exceptions would.

Maybe I have something setup wrong. I can wait until all this is released. Pytest does print where the exception was including a nice excerpt of the source. I can go there and place a breakpoint. It's less convenient but does work.

Below shows only User Uncaught Excepetion is checked but it's stopping at this handled exception:

try:
    from ._version import version as __version__
except ImportError:
    __version__ = "not-installed"

The option Raised Exceptions would be totally sufficient except there are many raised but handled exceptions in the code like this one.

debugpy-exception

@fabioz
Copy link
Collaborator

fabioz commented Jun 24, 2020

Humm, that's actually fixed in master (I just checked and there hasn't been another release after those fixes).

So, it seems that to use that you need to use the dev version, not just change that line... If you'd like, the steps for that are:

git clone https://github.com/microsoft/debugpy.git
cd debugpy
pwd

Then, in your launch config add:

"debugAdapterPath": "<pwd path>/src/debugpy/adapter"

(and then, uncomment #{"filter": "userUnhandled", "label": "User Uncaught Exceptions", "default": False} in the repo version as explained in #275 (comment)).

@pwinston
Copy link

pwinston commented Jun 24, 2020

Following those steps with debugAdapterPath set the debugger doesn't work at all, doesn't even hit regular breakpoints. Without debugAdapterPath it's as before where User Uncaught Exception behaves like Raised Exception.

I'm comforted this functionality is coming and I can wait! I think User Uncaught Exception is good general solution to this specific pytest issue.

@int19h
Copy link
Contributor

int19h commented Jun 24, 2020

You should be able to clone https://github.com/microsoft/debugpy directly as well - so long as it's master, it'll have the fix.

For "debugAdapterPath", make sure that the path is correct, and refers to the src/debugpy/adapter folder (i.e. not to the root of the repo!). E.g. I have mine cloned into C:\git\debugpy, so my developer config is:

"debugAdapterPath": "c:/git/debugpy/src/debugpy/adapter"

You can also test if the path is correct by running it like so:

python3 .../debugpy/src/debugpy/adapter

if you're seeing some JSON printed out, it's working.

@pwinston
Copy link

pwinston commented Jun 24, 2020

My path was right but fabioz->sourceref_267 did not produce any JSON with python3 .../adapter but switching to debugpy->master did produce JSON, and it somewhat works in the debugger!

However it doesn't quite work. User Uncaught Exceptions correctly does not break on handled exceptions like Raised Exceptions does, which is great. However when it does break it's breaking at the wrong place.

So if I have foo() -> bar() -> raise RuntimeError()
using Raised Exceptions it breaks exactly on the raise RuntimeError line, right where it's being raised, which is what you want.

With User Uncaught Exceptions however it breaks inside the pytest function, several stack frames up, after the other frames have exited. This is actually not very useful because the whole point is to examine the state of things where the exception is thrown.

For example below you can see it caught RuntimeError but it broke not where the RuntimeError was thrown. In this case viewer.add_image called something which called something which raised the RuntimeError. Getting closer!

user-uncaught

@int19h
Copy link
Contributor

int19h commented Jun 24, 2020

This is sort of by design, because we don't know that it's user-unhandled at the point where it happens - Python doesn't have two-pass exception model (i.e. it doesn't first scan the stack to figure out which except-block will handle, and only then unwind to it - it just unwinds frame by frame until it hits an except-block). So we just let it bubble up, until it gets to the boundary between user code and library code, and report it there.

We can synthesize the original stack, since all the info about it is contained in the exception traceback. This would allow you to inspect the variables in those frames. But if we showed that stack at the boundary, it would be lying about the location where the code is actually executing at that point. :/

@int19h
Copy link
Contributor

int19h commented Jun 24, 2020

@fabioz I wonder - since you're already looking at f_code to detect handlers in the same frame, could this be done for parent frames as well, walking the stack from the point of occurrence until it hits a library frame?

@fabioz
Copy link
Collaborator

fabioz commented Jun 24, 2020

Sorry about the copy/paste which was using my own repo (I fixed the comment to use the microsoft repo/master branch).

@int19h, that's possible, but also much worse for performance (because we'd need to do work for exceptions which are usually just skipped due to being library code and the detection would need to go up the stack many times).

I think that in this case we can just reconstruct the exception and show the original place -- even if we'd be lying about the location is actually executing -- maybe we can try to customize the printing of the exception to point where in the stack we're actually executing at that point to lie a bit less ;)

@int19h
Copy link
Contributor

int19h commented Jun 24, 2020

I don't think we need to do this for exceptions in library code at all, if "justMyCode" is set. We can just let it bubble out to user code, and only apply the check starting with the first user frame - this is the context that people would actually care about (and if they want to see the library context, they can always flip JMC setting).

Having to walk the stack multiple times as it bubbles through user frames is annoying, but there usually aren't that many - typically there's far more library/framework frames on the stack. And I don't think there's an expectation of exception propagation being super fast when debugging it, so some slowdown is acceptable here. Since we (will) expose "userUnhandled" in UI, if the slowdown is not acceptable, it can simply be disabled.

What concerns me is how it'd interact with iterators and async...

@pwinston
Copy link

Yeah it's too bad is doesn't work as hoped. I really don't think it's useful unless it breaks where the exception was raised. But "faking" the location sounds complicated. Worse I can imagine there is something the person wants to do in the debugger that simply doesn't work with the fake. I can't predict what. But it just seems like a case where the fake will be a lot of work, but also it will fall apart in some cases.

A totally different direction is in old versions of Visual Studio for C++ believe you had a filter by exception type. If you had that it would let you use Raised Exception in many cases because you can filter by exception type.

@pwinston
Copy link

pwinston commented Jun 24, 2020

In my example right now these exceptions are raised and caught before I get to my exception: ImportError, FileNotFoundError, KeyError, ZipImportError, AttributeError. So as long as my test was throwing any other exception, I could run with Raised Exception set filtered to only hit my exception type.

And if my exception was one of the above, I can hit the bogus exception and then hit continue. As long as I only have to do that a few times it's not horrible.

Yet another totally different solution is running with Raised Exception but no filters by exception type, but if every time you it an exception it added a line to some list like "`KeyError in foo.py at line 132" and then you could toggle off that very specific exception so you stop breaking there. Basically opt-in or opt-out to each exception raise.

Anyway just spit balling ideas. Maybe the User Uncaught Exception can be salvaged. But here I'm taking a step back saying the general problem which I think is "as a user, I want to break at certain exceptions not but not other ones". So what ways are there to delineate which exceptions I what to break at? By exception type, by file, by line number?

One thing I can do sometime is set a breakpoint near where the exception is going to happen, then turn on Raised Exception so I've skipped most of the false ones. But then if there are still handled exceptions they get in the way. Unless I can put the breakpoint even closer, etc. Maybe that whole thing could be automated somehow.

@pwinston
Copy link

Pytest folks ask can vscode set sys.breakpointhook? I’ve not heard of that one.

pytest-dev/pytest#7409

@fabioz
Copy link
Collaborator

fabioz commented Jun 24, 2020

Actually, fake is a bit of a strong word there (the stack is all there in the exception, it's just that we need to walk the tb.tb_next or tb.tb_frame.f_back to the proper place when showing the exception, which is what's already done on actual unhandled exceptions), so, showing the original raise location is actually trivial compared to other solutions...

@fabioz
Copy link
Collaborator

fabioz commented Jun 24, 2020

Pytest folks ask can vscode set sys.breakpointhook? I’ve not heard of that one.

pytest-dev/pytest#7409

It does set the breakpointhook, but that's more related to calling breakpoint() than stopping in an exception (but I think that what he's proposing is actually making the exception spill from pytest to be really unhandled -- that should work in the sense that the exception will be really unhandled and you should be able to get it in the debugger when dealing with unhandled exceptions, but if I understood properly, that will also stop your test suite -- which may be ok if you're running a single test).

@int19h
Copy link
Contributor

int19h commented Jun 24, 2020

We actually support filtering exceptions on a per-type basis - the problem is that VSCode doesn't wire that feature up (we could show them as exception filters I guess, but it would be a flat list, and not very manageable in VSCode UI). But if you use VS to debug Python, the Exceptions tool window there works exactly like it does for C++.

image

@int19h
Copy link
Contributor

int19h commented Jun 24, 2020

@fabioz It's fake in a sense that it does not accurately describe the real state of the debuggee. By the time we report the exception as user-unhandled, a bunch of code (e.g. all the __del__ and __exit__ methods, as well as finally blocks between the point where it was raised, and the point where it was reported) had already run, and may have mutated local variables and other state - but the stack synthesized from the exception traceback would look like that code is yet to run. This disparity between the stack and variable values would be confusing.

Furthermore, suppose the user decides to set a breakpoint inside finally after seeing the synthesized stack, and press F5 - they'd have a reasonable expectation of hitting it (on the assumption that unwinding is only about to happen), but it wouldn't be.

@fabioz
Copy link
Collaborator

fabioz commented Jun 24, 2020

I agree (but that's already the same as the regular unhandled exception, so, it may be a matter of tuning the expectation of the user and maybe producing a stack trace which makes that clearer).

@pwinston
Copy link

pwinston commented Jun 24, 2020

I think long term having per-type filters is a no brainer. Unless Microsoft is trying to hold back some features as Visual Studio only. It would help for many debugging situations unrelated to pytest or testing at all.

The pytest solution to make pytest no longer catch the exception works great: pytest-dev/pytest#7409 (comment). I wrapped it in an env var so I can toggle it on our off from my tests launch.config:

        {
            "name": "Debug Tests",
            "type": "python",
            "request": "test",
            "console": "integratedTerminal",
            "justMyCode": true,
            "env": {
                "_PYTEST_RAISE": "1"
            },
        }

So this is really perfect for me. I can run it either way. And when _PYTEST_RAISE is set it breaks exactly at the correct spot. After dragging in maybe 6 people across Stack Overflow and two GitHubs I think this is solved!

I put this solution as the answer to my stack overflow question.

@scottshambaugh
Copy link

This does not seem to be a working solution any more now that "test" is not a valid request type.

@danniesim
Copy link

@scottshambaugh conftest.py is no longer needed as we can activate "User Uncaught Exceptions" in VS Code under Breakpoints (filter) in the Run and Debug panel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants