OnError does not run for view errors when compiling to Release

[ayoung]

I have a commit on my fork that demonstrates that when you get an error in a view (in my case, a KeyNotFoundException), the OnError pipeline will not be run when it is compiled to Release mode. This works in Debug though.

In the example, if you run it in Release, you will notice that the exceptions fall through. It does run through the OnError, nor do any of the IStatusCodeHandlers get run

Is this a bug?

[ayoung]

Ok. I've narrowed it down to StaticConfiguration.DisableErrorTraces. With this true, which is the case in release mode, it will skip all the error handling and status code handlers.

[grumpydev]

It's not a bug, it's by design (albeit a contentious one) - the creation of the body of a Nancy response is deferred until the host asks for it, this is to avoid unnecessary work if the host never asks for it, or if the request is a HEAD request. In terms of views, this results in the second stage of the view engine's work,. where it actually plugs values into a compiled template, being done after the main pipeline has finished, which is why the error pipeline doesn't fire. This can be a bit of a pain to work with, especially when developing, so a wrapper was added to view responses that forces early execution of the body within the normal pipeline, but by default this is only used in debug mode (or to be exactly, it hangs off the DisableErrorTraces flag) because it's less efficient.

The wrapper is here:

https://github.com/NancyFx/Nancy/blob/master/src/Nancy/Responses/MaterialisingResponse.cs

And the processor that hooks it up is here:

https://github.com/NancyFx/Nancy/blob/master/src/Nancy/Responses/Negotiation/ViewProcessor.cs

If you want this behaviour all the time you can either leave error traces on, or replace the default ViewProcessor with your own and register it in the container.

[ayoung]

I can see why this is contentious. The interface/contract with regards to the OnError pipeline and IStatusCodeHandler is completely misleading especially when you expect these two features to work in release mode without giving it much thought. In order to have custom status code pages (404, 500s) on production (release), or to be able to react to view errors, the dev must to know about this design decision then consciously enable error traces. I'd argue that OnError and IStatusCodeHandler are most frequently used for these cases.

Ideally, Nancy would perform this "unnecessary work" when the app has been configured to use OnError or IStatusCodeHandler and not only rely on the static config / compilation configuration.

I also don't see mention of this in the documentation. Would you be ok with me adding this?

[grumpydev]

Believe me, you're preaching to the choir here :)

Updating the docs would be great, thanks - if you want to send a PR to make that materialisation happen on its own flag, rather than piggy backing on the error traces one, that'd be cool too.

[ayoung]

Great. Added notes in the wiki.