[WIP] Ensure template exceptions are not ignored #1179
Conversation
| @@ -541,11 +541,15 @@ def __init__(self, proc): | |||
| self.exceptions = {} | |||
| self.lock = threading.Lock() | |||
|
|
|||
| def remove_exception_break(self, ex_type=None, exception='BaseException'): | |||
There was a problem hiding this comment.
Might as well just make it ex_type='python' and skip the check inside, since strings are immutable.
| @@ -128,7 +128,9 @@ def should_stop_on_exception(self, frame, event, arg): | |||
| return False, frame | |||
|
|
|||
| if exception_breakpoint.ignore_libraries: | |||
| if not main_debugger.is_exception_trace_in_project_scope(trace): | |||
| if not main_debugger.is_exception_trace_in_project_scope(trace) and \ | |||
| main_debugger.plugin is not None and \ | |||
There was a problem hiding this comment.
I think this would fail in the following case:
- add django exception breakpoint
- mark to ignore exceptions in libraries
- non-django exception is raised
- exception would be shown to the user
The current problem is the following: if there's an exception breakpoint it always takes priority over plugin breakpoints (i.e.: if exception_breakpoint is not None...).
This is backwards, I think that the proper fix is checking if there are exception breakpoints and if there are, ask the plugins to handle it (i.e.: the main_debugger.plugin.exception_break currently in the else block) and if it's not handled by the plugins then go on to the current code.
So, we should have a test-case for this case (not sure if there is already) and the fix is something as:
# Plugins should have priority when handling exceptions so that an exception is not
# ignored if a plugin would handle it.
should_stop = False
if main_debugger.plugin is not None and main_debugger.plugin.has_exception_breaks():
result = main_debugger.plugin.exception_break(main_debugger, self, frame, self._args, arg)
if result:
should_stop, frame = result
if not should_stop:
exception_breakpoint = main_debugger.get_exception_breakpoint(
exception, main_debugger.break_on_caught_exceptions)
if exception_breakpoint is not None:
... current code to handle exception_breakpoint ...
There was a problem hiding this comment.
I tried this, it does not seem to work right. I see another problem. Template exceptions in django are not reporting in the django exception break context. Also the first frame is app.py in the test case. So there is something broken there.
Also, since it is not the first frame, just_reported is false. so django excpetion_break always returns None.
There was a problem hiding this comment.
So regular django-exception breakpoints aren't working properly? If something is broken in pydevd_plugins.django_debug.exception_break it should probably be fixed first then (do you want me to take a look at it?).
There was a problem hiding this comment.
Yes, please take a look at this. I will close this PR. I will reopen after the django part is fixed.
There was a problem hiding this comment.
Ok, created #1181 for that.
This reverts commit 7e51810.
karthiknadig
force-pushed
the
issue694
branch
from
66adbf0
to
90dc0b1
Compare
karthiknadig
changed the title
| @@ -229,14 +229,15 @@ def _NormPaths(filename): | |||
| try: | |||
| return NORM_PATHS_CONTAINER[filename] | |||
| except KeyError: | |||
| if filename.__class__ != str: | |||
| if filename.__class__ == str or (sys.version_info < (3, 0) and filename.__class__ == unicode): | |||
There was a problem hiding this comment.
@fabioz is there a better way to do 2.7 compat here?
There was a problem hiding this comment.
Humm, how did you get to this use case? (in general, this would be an error -- it should really be str on both -- i.e.: bytes on py2 and str on py3)
There was a problem hiding this comment.
running test_flask_template_exception_no_multiproc test on windows resulted in that assertion.
|
Closing this PR, I will create a separate PR with updated tests. The pydevd parts that need to be fixed have a separate task. |
Fixes #694