Cannot handle exceptions raised while running a response

[dcoutts]

In a customer's yesod application (using warp-2.0.x) we are getting a 500 response with "Something went wrong" as the error message. This is a little unusual since yesod can normally catch exceptions throw by calls to error.

A little digging reveals that this comes from the warp exception handler when running one or more requests on a socket.

serveConnection conn ii addr settings app =
    recvSendLoop (connSource conn th) `onException` send500

It's that send500 that produces the "Something went wrong".

While that's perhaps reasonable default behaviour for warp (don't want to leak info about the exception to an attacker) it is obvious that for development & deployment logging we want to be able to customise that.

So there are perhaps two bugs here:

• Yesod lets this exception slip through (unclear as we do not yet know where the exception is coming from)
• Altering the default behaviour of warp's top level exception handler appears to be extremely difficult.

The difficulty is that the exception is clearly being throw as the Response is run/unfolded rather than when it is constructed. So it's not enough to wrap a handler around the construction of the Response. I may be missing something but it doesn't look like it's possible to insert an exception handler inside a conduit (which is quite reasonable, it does safe resource management but that doesn't mean you should be able to fiddle with the running of a conduit from within one).

As a quick hack I've patched warp: http://lpaste.net/98599

This compiles and we're in the process of seeing if that does reveal more details about the exception being thrown in this case.

[snoyberg]

The send500 code is relatively new, and I'm not at all opposed to the idea of adding some kind of extensibility to it to allow for a user-generated message. What I'm more concerned about is why the exception slipped past Yesod. Was there a streaming response body being generated by any chance?

[kazu-yamamoto]

I don't understand this well at this moment. So, just a short comment. Rich error messages are necessary for debugging but they should not be delivered to users after deployment to prevent abusing. So, I think we need a way to customize the error messages.

[dcoutts]

What I'm more concerned about is why the exception slipped past Yesod.

Right, as a simple error in a page body does get caught fine. I'll know more once we find out what the exception is and where it's coming from (working on that...).

Was there a streaming response body being generated by any chance?

I don't know for sure yet, still investigating. I do suspect that the error occurs not in constructing the Response but in the unfolding/running of the response, however have yet to confirm.

@kazu-yamamoto yes absolutely. We want it reported in a dev situation but in a production situation we just want it logged, and no info leaked to a potential attacker.

[kazu-yamamoto]

I would propose to add a new field to Settings. What about settingsOnExceptionResponse?
@snoyberg if you kindly decide the field name, I will implement it.

[kazu-yamamoto]

@dcoutts Please try settingsOnExceptionResponse and exceptionResponseForDebug.

[snoyberg]

@kazu-yamamoto Those names sound find to me, thanks for implementing this!

[singpolyma]

settingsOnException should probably be changed to IO Response and then you only need one setting for both of these cases.

[kazu-yamamoto]

@singpolyma would you us the code?

[singpolyma]

@kazu-yamamoto Hmm, so, on another look at the code the default exception handler is pretty complex -- probably want to be able to replace the response without redoing all that, which makes me like your way better.

And the way it all ends up getting used... it's probably just added complexity to combine the two.

[kazu-yamamoto]

So, do you agree with introducing settingsOnExceptionResponse?

Since runSettingsConnectionMaker is really complex, I don't want to touch it by myself. I'm not sure that I completely understand the code and we have enough test cases.

[dcoutts]

@singpolyma @kazu-yamamoto I filed #226 which is very closely related here.

Summary: logging the exception is obviously the right thing to do, but we should not send a 500 response at all, and certainly not on warp shutdown. Possibly, possibly given that you're making it configurable you could have a way to opt-in to get a 500 response, but it should never be the default for development and certainly not production because its breaks infrastructure like proxies (and who knows what else).

[kazu-yamamoto]

With 8913a3d, we can send 500 when it is reasonable. However, users still cannot customize the handler. I think that 3f113e0 should also be merged with tweaks.

@dcoutts @snoyberg what do you think?

[snoyberg]

I agree.

[kazu-yamamoto]

OK. I will take care of this.

[kazu-yamamoto]

Done in 6de28ae.

@dcoutts if you think this patch is OK, please close this issue.

[dcoutts]

That looks good. Thanks very much Kazu!