Edgewall Software
Modify

Opened 16 years ago

Closed 16 years ago

#3328 closed defect (fixed)

Error display regression 0.9->0.10 (no more HTML errors, just text)

Reported by: maxb1@… Owned by: Christian Boos
Priority: normal Milestone: 0.10
Component: general Version: devel
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

In 0.9.x, an error (e.g. accessing an URL for which there is no registered handler) results in a pleasant HTML display with the Trac navigation menus and the error displayed in as red boxed message.

In 0.10dev, the same situation results in a plain-text python traceback.

Similarly, internal errors such as the one #2611 are displayed within the Trac interface with 0.9.x, but with 0.10dev, an Apache-standard 500 Internal Server Error page is shown.

Attachments (4)

3328.patch (1.1 KB ) - added by Emmanuel Blot 16 years ago.
Patch against trunk [3491]
trap_early_errors-r3491.diff (3.0 KB ) - added by Christian Boos 16 years ago.
Catch early errors in request dispatching, and re-raise them once the HDFWrapper is created.
trap_early_errors-r3549.diff (3.5 KB ) - added by Christian Boos 16 years ago.
Second attempt, a bit more robust.
trap_early_errors_exc_info.diff (3.5 KB ) - added by Matthew Good 16 years ago.
use "sys.exc_info()" for capturing early exceptions

Download all attachments as: .zip

Change History (15)

comment:1 by maxb1@…, 16 years ago

Version: 0.9.5devel

comment:2 by Emmanuel Blot, 16 years ago

I'm attaching a patch for the "no handler matched request" error (404).
I do not commit it to the trunk, as I'm not sure that's the way to solve it.

by Emmanuel Blot, 16 years ago

Attachment: 3328.patch added

Patch against trunk [3491]

in reply to:  description comment:3 by Emmanuel Blot, 16 years ago

Similarly, internal errors such as the one #2611 are displayed within the Trac interface with 0.9.x, but with 0.10dev, an Apache-standard 500 Internal Server Error page is shown.

Were they? I don't think that all kinds of error can be catched and displayed within the Trac interface. For example, #2611 occurs at an early stage in the request handling management. It is likely that the initialization sequence has been changed for 0.10

in reply to:  2 comment:4 by Christian Boos, 16 years ago

Milestone: 0.10

Replying to eblot:

I'm attaching a patch for the "no handler matched request" error (404).

It's OK for the "no handler matched request" error, but I've generalized it to handle other early errors: attachment:trap_early_errors-r3491.diff

by Christian Boos, 16 years ago

Catch early errors in request dispatching, and re-raise them once the HDFWrapper is created.

comment:5 by Matthew Good, 16 years ago

I'm concerned about attachment:trap_early_errors-r3491.diff catching all exceptions and saving them for later. If an exception occurs in one of the earlier blocks it may leave local variables undefined which would cause more exceptions in later blocks until early_error is rethrown. I think the 404 errors should be fixed for 0.10, but unless there's a more clean way to deal with the other exceptions I'd rather not commit that second patch this close to the 0.10 release.

comment:6 by Christian Boos, 16 years ago

Well, I'll make another attempt…

by Christian Boos, 16 years ago

Second attempt, a bit more robust.

comment:7 by Christian Boos, 16 years ago

Keywords: review added
Owner: changed from Jonas Borgström to Christian Boos
Status: newassigned

Ok, here's my second attempt: attachment:trap_early_errors-r3549.diff

How this works:

  1. trap and save possible early error during selection of handler
  2. eventually create an early HTTPNotFound error if previous step succeeded but no handler was found
  3. user information:
    • if no early error, proceed normally but trap and save possible early error at that stage
    • else, setup for anonymous request (can't fail)
  4. prepare the HDF data for the template
    • if an error occurs during the HDF setup, clear the HDF. This will revert to plain text error reporting.
      • if there was already an early error saved, report that early error (the error during the HDF setup will get ignored, but we can imagine there was one by seeing the plaintext report instead of the templatized one)
      • else, the error during HDF setup gets reported. Example of such errors: problems in INavigationContributor plugins
  5. if there was an early error, it gets re-raised at that point. If everything went well in 4., it will be reported using error.cs, otherwise as plaintext.

Please review.

comment:8 by Christopher Lenz, 16 years ago

I think this is mostly okay, but I have to say the whole approach of trying to avoid the session and HDF stuff, which introduced the need for this “early error” story, seems overly complex, and we really need something nicer for the next release.

Anyway, in the meantime, the patch should do IMHO. Would also like to hear what Matt thinks, though.

in reply to:  8 comment:9 by Christian Boos, 16 years ago

Replying to cmlenz:

… trying to avoid the session and HDF stuff …

While I agree there could probably be a simpler approach to do this, I remind you that this was introduced for not having to do those relatively expensive initializations for every /chrome request. A normal Trac request ends up in a dozen secondary requests for images, css, javascript. Each of them used to trigger that whole setup sequence, for nothing, if not for making sure you'd trigger a database is locked error… (due to the authenticate step, setup of the PermissionCache and setup of the Session object).

So this has to be kept in mind when redesigning it, maybe by having a different category of request dispatchers.

by Matthew Good, 16 years ago

use "sys.exc_info()" for capturing early exceptions

comment:10 by Matthew Good, 16 years ago

Ok, I've attached a minor alteration of cboos' last patch using sys.exc_info() for getting the excpetion information, which seems a little cleaner to me.

comment:11 by Christian Boos, 16 years ago

Keywords: review removed
Resolution: fixed
Status: assignedclosed

mgood's version applied in r3568. Thanks for the review!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos 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.