Better stacktrace

[vlyon]

• Cleaner, clearer display of the context of an error. Uses table instead of pre and shows 5 lines of context instead of almost 3. (bug fixed)
• Constructs the stack trace from the actual location of the error instead of where D2 calls response_internal_error which makes the stacktraces completely useless currently.
• Cleaner, clearer display of the stacktrace and includes the arguments passed. (very useful)

[bigpresh]

This looks pretty sane and useful to me - thanks for the contribution! Would like another core dev to review as well though, in particular the addition of Devel::StackTrace as a dep. Personally I have no problem with it, it does its job well, and has a short and reliable chain of deps itself: http://deps.cpantesters.org/?module=Devel%3A%3AStackTrace&perl=5.22.0&os=any+OS

[vlyon]

Please note, there is one thing this PR doesn't address:
Changes to the css in share/skel/public/css/error.css won't get picked up by apps that have already been created. This is not really a flaw of this PR but rather of the fact that we include the css in a separate file. To solve this, I believe the css should be moved into the error page as it is rendered.
Comments?

Hi @bigpresh, Devel::StackTrace is a dependency of Plack, so it's already in D2. 😃

[xsawyerx]

@vlyon Good stuff. I'm not sure about the evals or the local $SIG{'__DIE__'}. I'd also like to try it out first.

Regarding the css file: we can also use File::ShareDir to save it locally and use it. The problem with it is that you will not be able to serve your own version of it. The benefit of scaffolding a static file is that you can change it as much as you want later.

Maybe we can introduce a way in dancer2 to update your static files if they're a known older version?

[vlyon]

@xsawyerx Most of the code has been "borrowed" from Miyagawa's Plack::Middleware::StackTrace

The SIG handler is the only dependable way that I know to generate a stack-trace at the point of the error.

A few points of interest from that page:

The stack trace is also stored in the environment as a plaintext and HTML under the key plack.stacktrace.text and plack.stacktrace.html respectively, so that middleware further up the stack can reference it.
This middleware is enabled by default when you run plackup in the default development mode.

I would be really good to make use of this middleware (when enabled) instead of bringing our own. It would require some big changes though to the way errors are thrown and caught.
For now though, this patch will provide a nice intermediate.

[vlyon]

@xsawyerx Regarding error.css:

Maybe we can introduce a way in dancer2 to update your static files if they're a known older version?

We don't want to have to create upgrade scripts for D2, this'll add really unnecessary complexity, prone to many bugs & issues.

we can also use File::ShareDir to save it locally

No, this just defeats the whole purpose of providing a static file. Like you said, the file then can't be customised.

There is only one place where it is being used. This is in D2::Core::Error.

I don't think this should be in a separate file, the CSS should be served along with the page (there isn't very much) or even in the HTML. If someone wants to customise the look, there are many ways to override the way errors are displayed. But if we want to change the way it looks, and the CSS is in a static file, then we can't, we're stuck!

[veryrusty]

On 30/10/2015 6:45 pm, Vernon wrote:

@xsawyerx https://github.com/xsawyerx Regarding |error.css|:

Maybe we can introduce a way in |dancer2| to update your static
files if they're a known older version?

We don't want to have to create upgrade scripts for D2, this'll add
really unnecessary complexity, prone to many bugs & issues.

Update <> Upgrade. It also shouldn't be automatic; an app developer
should review any changes.

I like the approach Catalyst has when updating its scripts; if the file
already exists then it writes out the newer version with a .new extension.

--veryrusty

[xsawyerx]

I'm still quite scared of this change and don't feel comfortable adding it as it, at least right now.

Other options:

• Give up.
• Convince me I'm wrong (it happens!). (I'm willing to revise everything again.)
• Introduce incremental changes which might end up here.

I hope the first doesn't happen. I'm open to the second. And I think the last is the most likely one to work.

What do you guys say?

[vlyon]

Hi @xsawyerx,

I'm glad you don't want to give up on it.
I'm also willing to split it into 2 smaller PRs.

1. Delete error.css and move all the style into the generated HTML.
   It doesn't make sense to have the error page split between a static css file and generated HTML.
   This won't break existing apps, as it only removes the file from newly created apps.
   It will then pave the way to improve the error page.
2. Actually implement a stacktrace at the point of the error instead of the current useless behaviour.
   Possibly consider making it optional (config setting).

[SysPete]

On 29/03/16 18:45, Vernon Lyon wrote:

Hi @xsawyerx https://github.com/xsawyerx,

I'm glad you don't want to give up on it.
I'm also willing to split it into 2 smaller PRs.

Delete |error.css| and move all the style into the generated HTML.
It doesn't make sense to have the error page split between a
static |css| file and generated HTML.
This won't break existing apps, as it only removes the file from
newly created apps.
It will then pave the way to improve the error page.

This won't work for paranoid types (like me) who use
Content-Security-Policy headers to block CSS directives within HTML
pages. I'd prefer to keep things in error.css.

[vlyon]

Ok, well that brings us back to using the patch as it is.

Can we at least use the first commit 0e05708 in this PR? It doesn't really need the style changes in the css file, it would just look a bit nicer. It's mainly just a cosmetic change and a tiny bugfix anyway.

Also my first commit actually removes inline CSS, so paranoids like @SysPete should be happy too.

Once that's there I'll look at creating a new PR with the other changes if you like?

[vlyon]

Actually lets break this into 2 PRs.

1. Error formatting Cleaner default error page #1146
2. Coming soon ...

[SysPete]

I've rebased commit 6a4b8d6 against master in branch https://github.com/PerlDancer/Dancer2/tree/vlyon-better-stack-trace and am looking at what is needed to integrate this with Dancer::Core::App::$EVAL_SHIM.

[SysPete]

Now I've played with this a little I think this PR should be closed: it does improve the stack display in the error page when running in dev mode but it does nothing for logging and exception handling in general. I think it is better to concentrate on a more complete solution as per #765

[vlyon]

True, this change does nothing for logging, and yes, it makes no changes to exception handling.
What it does do is change the displayed stack-trace, which is currently incorrect.

This happens because of the eval in D2::Core::App::_dispatch_route, which catches the error and passes this to D2::Core::Error which then builds the stack trace, not from the actual location of the error, but from where D2::Core::Error was called. This makes the stack-trace completely useless.

Using Throwable or some other complete solution would be great, but it will still need (and will not be affected by) this change to ensure that stack-traces are generated from the actual error.

@SysPete Thanks for the rebase above, I've taken those changes and adjusted them slightly, 7448eec

[SysPete]

@vlyon agree: we still need to catch the correct error. Now I just need a few more tuits.

[vlyon]

Let me know if I should create a new PR from that commit. 7448eec

[cromedome]

@SysPete have your tuits returned? ;)

In all seriousness, how can we get this one across the finish line?

[SysPete]

my current $work2 D2 stack includes a hack along these lines:

# Dancer2 does not yet have a way to sensibly rethrow error objects
# to Plack middleware, so catch Foo::Throwable here.

around "Dancer2::Core::App::response_internal_error" => sub {
    my ( $orig, $self, $error ) = @_;

    if ( $error->$_isa('Foo::Throwable') {
...

so I'm more interested in #765 than messing with current exceptions right now. Sadly tuits are still in rather short supply. If I manage to catch up with $work1 (TS), then I'll have another look at this PR in a few weeks