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

[maxb1@…]

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.

[Emmanuel Blot]

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.

[Emmanuel Blot]

Patch against trunk [3491]

[Emmanuel Blot]

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

[Christian Boos]

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​

[Christian Boos]

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

[Matthew Good]

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.

[Christian Boos]

Well, I'll make another attempt…

[Christian Boos]

Second attempt, a bit more robust.

[Christian Boos]

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.

[Christopher Lenz]

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.

[Christian Boos]

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.

[Matthew Good]

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

[Matthew Good]

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.

[Christian Boos]

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