Edgewall Software
Modify

Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#12385 closed defect (fixed)

Trap NotImplementedError

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.11
Component: general Version:
Severity: normal Keywords: exception
Cc: Branch:
Release Notes:

NotImplementedError is trapped and a system message is presented to the user rather than the error reporting page.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

Raising a NotImplementedError exception results in the following page:

Note that this standard error screen suggests that the user might create a ticket. If the developer is explicitly raising a NotImpementedError, we probably don't want users reporting these issues (#11611). Ideally the NotImplementedError would have an explanation, perhaps even a link to the ticket in which the feature/method will be implemented. That may be applicable to the scenario that led to the screen capture.

Traceback (most recent call last):
  File "/usr/local/virtualenv/1.0-stable/lib/python2.7/site-packages/trac/web/main.py", line 554, in _dispatch_request
    dispatcher.dispatch(req)
  File "/usr/local/virtualenv/1.0-stable/lib/python2.7/site-packages/trac/web/main.py", line 247, in dispatch
    resp = chosen_handler.process_request(req)
  File "/usr/local/virtualenv/1.0-stable/lib/python2.7/site-packages/trac/versioncontrol/web_ui/log.py", line 271, in process_request
    for cpath, kind, chg, bpath, brev in changeset.get_changes():
  File "/usr/local/virtualenv/1.0-stable/lib/python2.7/site-packages/trac/versioncontrol/api.py", line 1244, in get_changes
    raise NotImplementedError
NotImplementedError: 

We could add a TracNotImplementedError:

class TracNotImplementedError(TracError, NotImplementedError):

Or we could trap NotImplementedError where TracError is trapped.

Attachments (3)

Screen Shot 2016-03-04 at 22.41.55.png (43.5 KB ) - added by Ryan J Ollos 5 years ago.
Screen Shot 2016-03-17 at 23.44.41.png (141.3 KB ) - added by Ryan J Ollos 5 years ago.
Screen Shot 2016-03-21 at 22.09.26.png (26.8 KB ) - added by Ryan J Ollos 5 years ago.

Download all attachments as: .zip

Change History (27)

by Ryan J Ollos, 5 years ago

comment:1 by Ryan J Ollos, 5 years ago

Description: modified (diff)
Owner: set to Ryan J Ollos
Status: newassigned

Proposed changes in log:rjollos.git:t12385_trap_not_implemented. I think [12cdd650/rjollos.git] could be adapted and applied to 1.0-stable.

I wanted to test with MercurialPlugin but haven't managed to install due to issue noted in comment:26:ticket:7346.

comment:2 by Jun Omae, 5 years ago

Functional tests has 2 failures. How about simply raising a TracError instead of NotImplementedError?

python trac/tests/functional/__init__.py 
..F.........F............................................................................................................................................................................................................
======================================================================
FAIL: runTest (trac.tests.functional.testcases.TestErrorPage)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/12cdd65075279a8db3acc4d920ed8fd0b3a2cbb1/py27-sqlite/trac/tests/functional/testcases.py", line 89, in runTest
    tc.find('<form class="newticket" method="get" '
  File "/run/shm/12cdd65075279a8db3acc4d920ed8fd0b3a2cbb1/py27-sqlite/trac/tests/functional/better_twill.py", line 229, in better_find
    (to_unicode(e), filename))
TwillAssertionError: no match to '<form class="newticket" method="get" action="http://trac-hacks.org/newticket">' at file:///run/shm/12cdd65075279a8db3acc4d920ed8fd0b3a2cbb1/py27-sqlite/testenv/trac/log/TestErrorPage.html

======================================================================
FAIL: runTest (trac.tests.functional.testcases.RegressionTestTicket11434)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/12cdd65075279a8db3acc4d920ed8fd0b3a2cbb1/py27-sqlite/trac/tests/functional/testcases.py", line 342, in runTest
    tc.find('<form class="newticket" method="get" '
  File "/run/shm/12cdd65075279a8db3acc4d920ed8fd0b3a2cbb1/py27-sqlite/trac/tests/functional/better_twill.py", line 229, in better_find
    (to_unicode(e), filename))
TwillAssertionError: no match to '<form class="newticket" method="get" action="http://trac-hacks.org/newticket">' at file:///run/shm/12cdd65075279a8db3acc4d920ed8fd0b3a2cbb1/py27-sqlite/testenv/trac/log/RegressionTestTicket11434.html

----------------------------------------------------------------------
Ran 217 tests in 267.780s

FAILED (failures=2)

Also, I don't think we should use raise e! The stack trace of exception would be changed by re-raising.

>>> import traceback
>>> try:
...   try:
...     raise ValueError('***')
...   except ValueError as e:
...     traceback.print_exc()
...     raise e
... except ValueError as e2:
...   traceback.print_exc()
...
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
ValueError: ***
Traceback (most recent call last):
  File "<stdin>", line 6, in <module>
ValueError: ***

If we indeed re-raise a caught exception, I think we should use err = sys.exc_info() and raise err[0], err[1], err[2].

Last edited 5 years ago by Jun Omae (previous) (diff)

in reply to:  2 comment:3 by Ryan J Ollos, 5 years ago

Replying to Jun Omae:

If we indeed re-raise a caught exception, I think we should use err = sys.exc_info() and raise err[0], err[1], err[2].

I was thinking that's an older form of exception raising according to PEP8, even though it's doesn't explicitly call out this 3 expression form.

When raising an exception in Python 2, use raise ValueError('message') instead of the older form raise ValueError, 'message'

I guess we would normally use raise:

>>> try:
...   try:
...     raise ValueError('**')
...   except ValueError as e:
...     traceback.print_exc()
...     raise
... except ValueError as e2:
...   traceback.print_exc()
... 
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
ValueError: **
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
ValueError: **
>>> try:
...   try:
...     raise ValueError('**')
...   except ValueError as e:
...     traceback.print_exc()
...     err = sys.exc_info()
...     raise err[0], err[1], err[2]
... except ValueError as e2:
...   traceback.print_exc()
... 
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
ValueError: **
Traceback (most recent call last):
  File "<stdin>", line 3, in <module>
ValueError: **

but that won't work in this case because of the inner exception. I guess there is nothing wrong with the 3 expression form and it is appropriate for this use case.

in reply to:  2 ; comment:4 by Ryan J Ollos, 5 years ago

Replying to Jun Omae:

Functional tests has 2 failures. How about simply raising a TracError instead of NotImplementedError?

Do you mean, raise TracError instead of TracNotImplementedError, or that plugins should modify their code to raise TracError rather than NotImplementedError?

comment:5 by Ryan J Ollos, 5 years ago

I committed the refactoring in [14604], with changes suggested in comment:2. I'll rework the other changes.

comment:6 by Jun Omae, 5 years ago

Looking [14604], if no reason, we should use except ExceptionName as e rather than if isinstance(e, ExceptionName).

  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index d30956db8..cbca8b459 100644
    a b class RequestDispatcher(Component):  
    275275                self._post_process_request(req)
    276276        except RequestDone:
    277277            raise
    278         except Exception as e:
     278        except PermissionError as e:
     279            raise HTTPForbidden(e)
     280        except ResourceNotFound as e:
     281            raise HTTPNotFound(e)
     282        except TracError as e:
     283            raise HTTPInternalError(e)
     284        except Exception:
    279285            # post-process the request in case of errors
    280286            err = sys.exc_info()
    281287            try:
    282288                self._post_process_request(req)
    283289            except RequestDone:
    284290                pass
    285             except Exception as e2:
     291            except Exception as e:
    286292                self.log.error("Exception caught while post-processing"
    287293                               " request: %s",
    288                                exception_to_unicode(e2, traceback=True))
    289             if isinstance(e, PermissionError):
    290                 raise HTTPForbidden(e)
    291             elif isinstance(e, ResourceNotFound):
    292                 raise HTTPNotFound(e)
    293             elif isinstance(e, TracError):
    294                 raise HTTPInternalError(e)
    295             else:
    296                 raise err[0], err[1], err[2]
     294                               exception_to_unicode(e, traceback=True))
     295            raise err[0], err[1], err[2]
    297296
    298297    # ITemplateProvider methods
    299298
Last edited 5 years ago by Jun Omae (previous) (diff)

by Ryan J Ollos, 5 years ago

comment:7 by Ryan J Ollos, 5 years ago

Previously though, self._post_process_request would be called even in case of PermissionError, ResourceNotFound and TracError. It appears that wouldn't be the case with proposed changes. I might be wrong about that. The old behavior was very confusing with so many nested control structures.

I think it's a better easier to see changes in this diff view.

in reply to:  7 ; comment:8 by Jun Omae, 5 years ago

Replying to Ryan J Ollos:

Previously though, self._post_process_request would be called even in case of PermissionError, ResourceNotFound and TracError. It appears that wouldn't be the case with proposed changes. I might be wrong about that. The old behavior was very confusing with so many nested control structures.

I think it's a better easier to see changes in this diff view.

Yeah, removing nested try/except in [14604] looks good but I dislike isinstance(e, ExceptionName) in except block. Please ignore the diff in comment:6, sorry.

Last edited 5 years ago by Jun Omae (previous) (diff)

in reply to:  4 ; comment:9 by Jun Omae, 5 years ago

Replying to Ryan J Ollos:

Replying to Jun Omae:

Functional tests has 2 failures. How about simply raising a TracError instead of NotImplementedError?

Do you mean, raise TracError instead of TracNotImplementedError, or that plugins should modify their code to raise TracError rather than NotImplementedError?

I misunderstood. I agree adding NotImplementedError. Reconsidering, I think the Trac administrators want to know where does the NotImplementedError come from. How about logging last entry of the stacktrace?

  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index d30956db8..3be58af91 100644
    a b import pkg_resources  
    3030from pprint import pformat, pprint
    3131import re
    3232import sys
     33import traceback
    3334
    3435from genshi.builder import tag
    3536from genshi.output import DocType
    class RequestDispatcher(Component):  
    288289                               exception_to_unicode(e2, traceback=True))
    289290            if isinstance(e, PermissionError):
    290291                raise HTTPForbidden(e)
    291             elif isinstance(e, ResourceNotFound):
     292            if isinstance(e, ResourceNotFound):
    292293                raise HTTPNotFound(e)
    293             elif isinstance(e, TracError):
     294            if isinstance(e, NotImplementedError):
     295                tb = traceback.extract_tb(err[2])[-1]
     296                self.log.warning('%s caught from %s:%d in %s: %s',
     297                                 e.__class__.__name__, tb[0], tb[1], tb[2],
     298                                 to_unicode(e) or '(no message)')
     299                raise HTTPInternalError(TracNotImplementedError(e))
     300            if isinstance(e, TracError):
    294301                raise HTTPInternalError(e)
    295             else:
    296                 raise err[0], err[1], err[2]
     302            raise err[0], err[1], err[2]
    297303
    298304    # ITemplateProvider methods
    299305
Last edited 5 years ago by Jun Omae (previous) (diff)

in reply to:  8 comment:10 by Ryan J Ollos, 5 years ago

Replying to Jun Omae:

Yeah, removing nested try/except in [14604] looks good but I dislike isinstance(e, ExceptionName) in except block.

Yeah, I dislike it as well. So far I didn't see a better way, but I'll continue to look for ideas for additional refactoring.

in reply to:  9 comment:11 by Ryan J Ollos, 5 years ago

Replying to Jun Omae:

I misunderstood. I agree adding NotImplementedError. Reconsidering, I think the Trac administrators want to know where does the NotImplementedError come from. How about logging last entry of the stacktrace?

That's a good idea. I've prepared changes:

comment:12 by Jun Omae, 5 years ago

rjollos.git@t12385_trap_not_implemented_1.0-stable.1 has a syntax error for Python 2.5.

python ./trac/test.py --skip-functional-tests 
/run/shm/e51b3da7994bf506a2f6d42e4d9d7c48b204e28a/py25-sqlite/trac/web/tests/main.py:241: Warning: 'as' will become a reserved keyword in Python 2.6
Traceback (most recent call last):
..
  File "/run/shm/e51b3da7994bf506a2f6d42e4d9d7c48b204e28a/py25-sqlite/trac/web/tests/main.py", line 241
    except HTTPInternalError as e:
                              ^
SyntaxError: invalid syntax
make: *** [unit-test] Error 1

comment:13 by Jun Omae, 5 years ago

When generating navigation items from INavigationContributor components, exceptions from the components is logged without stacktrace. See tags/trac-1.0.10/trac/web/chrome.py@:811-816#L809. I think the behavior makes difficult debugging/trouble shooting.

The logging last entry of the stacktrace might help the debugging/trouble shooting in the case. Thoughts?

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

in reply to:  13 ; comment:14 by Ryan J Ollos, 5 years ago

Replying to Jun Omae:

The logging last entry of the stacktrace might help the debugging/trouble shooting in the case. Thoughts?

Logging the stacktrace is a good idea. The Error with navigation contributor is raised on the mailing list fairly often and it's difficult to debug the problem.

comment:15 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

I addressed the errors in comment:12 (thanks!), then comment:11 changes committed to 1.0-stable in [14618], merged to trunk in [14619].

I'll leave the ticket open to address comment:13, unless we want to have a different ticket for that.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

by Ryan J Ollos, 5 years ago

comment:16 by Ryan J Ollos, 5 years ago

Similar to comment:13, I ran into an issue today in which the following error was displayed:

I think the issue is caused by SearchAttachmentsPlugin. This is what was logged:

2016-03-21 11:54:06,157 Trac[main] WARNING: [96.93.126.65] HTTPInternalError: 500 Trac Error (Genshi UnicodeDecodeError error while rendering template 'search.html', line 63, char 77)

This Trac instance has:

[trac]
use_chunked_encoding = false

Is there a reason that the stacktrace isn't printed for this case?: tags/trac-1.0.10/trac/web/chrome.py@:1130-1133#L1108

The stacktrace is printed when use_chunked_encoding?: tags/trac-1.0.10/trac/web/chrome.py@:1188-1190#L1165

in reply to:  14 comment:17 by Ryan J Ollos, 5 years ago

Replying to Ryan J Ollos:

Replying to Jun Omae:

The logging last entry of the stacktrace might help the debugging/trouble shooting in the case. Thoughts?

Logging the stacktrace is a good idea. The Error with navigation contributor is raised on the mailing list fairly often and it's difficult to debug the problem.

gmessage:trac-users:rDT7ljOFcm4/gcgX3vqeCwAJ is one such case, from what I remember.

in reply to:  16 comment:18 by Jun Omae, 5 years ago

Replying to Ryan J Ollos:

Is there a reason that the stacktrace isn't printed for this case?: tags/trac-1.0.10/trac/web/chrome.py@:1130-1133#L1108

That code is introduced in [7838] (#7959). Perhaps, the reason is the printing stacktrace doesn't help because the UnicodeError is raised from genshi/core.py in most cases if the UnicodeError is raised while rendering genshi template.

However, it would be good to log the stacktrace before raising a converted TracError.

The stacktrace is printed when use_chunked_encoding?: tags/trac-1.0.10/trac/web/chrome.py@:1188-1190#L1165

I forget the reason…. Maybe, I considered we should log the error with stacetrace since caught exception isn't converted to TracError.

comment:19 by Ryan J Ollos, 5 years ago

Documented TracNotImplementedError in TracDev/Exceptions@17. I should probably add a docstring to the TracNotImplementedError class.

in reply to:  16 comment:20 by Ryan J Ollos, 5 years ago

Replying to Ryan J Ollos:

Similar to comment:13, I ran into an issue today in which the following error was displayed:

Might not be directly related to what we are discussing in this ticket, but as a side note I changed use_chunked_encoding to true and no longer see the error described in comment:16.

in reply to:  19 comment:21 by Ryan J Ollos, 4 years ago

Replying to Ryan J Ollos:

Documented TracNotImplementedError in TracDev/Exceptions@17. I should probably add a docstring to the TracNotImplementedError class.

Documented in [14670], merged to trunk in [14671].

comment:22 by Ryan J Ollos, 4 years ago

Resolution: fixed
Status: assignedclosed

comment:13, and possibly comment:16, can be addressed in #12428.

comment:23 by Ryan J Ollos, 4 years ago

Additional changes in [14672,14674,14676] on 1.0-stable, and [14673,14675,14677] on trunk. Sorry for the noise, I'll be more careful in the future.

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:24 by Ryan J Ollos, 3 years ago

See #12818 for possible regression in r14604.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.