@tamoyal Did you set :show_exceptions to :after_handler or something else?

@mwpastore I only used true/false values for the above

There's your problem. 😄 From the README (emphasis mine):

The error handler is invoked any time an exception is raised from a route block or a filter. But note in development it will only run if you set the show exceptions option to :after_handler:

set :show_exceptions, :after_handler

@mwpastore Gotcha. Seems a little strange to me but hey, if I'm the only one who thinks that, the community has spoken. I would just think it's good default behavior to not eat exceptions in development. I was also looking specifically at the configuration settings docs which does not mention this. Do you think that's worth an update?

Maybe I misunderstood your original issue. Exceptions should always show in the logs. If they're not showing, something else is wrong. I want to focus on this sentence you wrote here:

koala [..] can raise exceptions with an http status code of 400 if an OAuth token is invalid

This is not accurate. Koala can raise exceptions, but it can't set HTTP status codes. You would have to catch the exception (either inline with the call to Koala, or in an error handler) and set the status code yourself. Uncaught exceptions manifest as 500 Internal Server Error.

Maybe you can share a sample application that demonstrates what you want to happen, and what's happening instead?

Seems a bit strange the code would pier into any exception and decide how to behave based on it responding to http_status which at least some Koala exceptions do but TBH, I don't know the Sinatra codebase well enough (not opposed to helping out though). I put together an app for you which shows a 400 error getting eaten. Let me know if this helps - https://github.com/tamoyal/wtfrank

You appear to have found some undocumented behavior, or what I suspect is a private API. Sinatra does indeed look to see if an exception responds to :http_status and uses its value in the response if it does. I think it's pure coincidence that Koala exceptions respond to this method; those HTTP statuses correspond to the request from your server to Facebook and should (probably) not be used as the status reported to your user. You'll want to manually catch Koala exceptions in your Sinatra code and halt with your own error messages (or raise your own exceptions).

I certainly want to catch those exceptions but let's say Koala updates their codebase and introduce a new exception. If I don't know about that introduction, it could be eaten causing a hard to track down bug. Again, not knowing the Sinatra code very well, do you think the class of the exception should be checked so Sinatra doesn't eat exceptions from 3rd party libraries? I'm not sure the best thing to do here.

Koala exceptions are organized into a hierarchy, and any new exceptions will be added within the existing hierarchy, so as long as you're catching near the top of the hierarchy it should be fine. If Koala releases a new major version you'll probably want to scan the changelog and rerun your test suite—but you should be doing that anyway!

Something like this:

def events
  graph = Koala::Facebook::API.new(FB_TOKEN, FB_APP_SECRET)
  graph.get_connections("me", "events")
rescue Koala::Facebook::APIError
  halt 403, "Please try again later" # or whatever
rescue Koala::KoalaError
  halt 503, "Unknown error" # or whatever
end

I'm cool with the resolution being that I should stay up to date on that level of detail about a library I'm using. I initially thought you'd consider this to be a flaw in the Sinatra design. Close?

I'm just a Sinatra user so it might be worth leaving this issue open to see what the actual Sinatra maintainers have to say. There might be a way to refactor this behavior such that it doesn't interfere with Koala, but there's no telling what other libraries out there (such as Sinatra extensions) might rely on this behavior. I don't consider it a flaw in Sinatra as much as a weird coincidence that is going to be tricky to untangle!

Gotcha. I agree it's best to at least let them see this and determine if it's a flaw. Thanks!

I have no idea about Koala, but this seems to be you're running into surprising, yet intended, behavior.

Could you confirm that enabling this setting fixes your issue?

Thanks @tamoyal

@zzak Yo! Amnesia has already set in on this issue but I remember it being more of an aggressive duck typing issue that a setting doesn't resolve but catching the right exceptions from the library did. You may want to take a look at the sample project I posted

Thanks @tamoyal. Please close the issue if there's nothing further. Have a great day!

@mwpastore I still think it's best for a maintainer to look at this and close the issue as you suggested. I mean...we can always close it and wait for someone else to complain. It certainly does not seem to be critical

Same exact issue with this library - https://github.com/tbalthazar/onesignal-ruby

I had to look at the source code and specifically catch their errors (luckily I knew to do this) because Sinatra was eating them.

@tamoyal Thanks Tony. If nothing else it's good to have this thread here being indexed by Google for future seekers of wisdom.

@zzak How would you feel about adding a configurable e.g. :smart_exceptions that defaults to true. When set to false, Sinatra will only use the #http_status of an Exception if its class is in the Sinatra namespace. (Or something along these lines, e.g. :ignore_exception_http_status instead.) This way the current behavior is preserved, and heavy users of Koala, OneSignal, and other gems have a workaround if they want those exceptions to bubble up like any others?

@mwpastore For sure - I think this will cause head banging the first time you experience this. Why are any exceptions not in the Sinatra namespace caught? I'm confused about that use case.

@tamoyal I don't know, but I can guess: duck typing is the preferred pattern, so that's what was used; and it probably simply didn't occur to whoever implemented this functionality originally that libraries making outbound HTTP calls would want/need to raise exceptions that respond to #http_status as well.

Couple things, first what is the use case for this?

I'm not sure I understand the behavior you're suggesting, perhaps it would be easier with code.

Also, any behavior change we introduce at this point should be set behind a flag which defaults to the original behavior. Especially the area around exception handling in Sinatra is rather sticky, and could use an overhaul anyways so I'm less motivated to add more to it.

The use case is some library throws an exception that happens to respond to http_status and sinatra eats it so you have no idea what happened. Code to show this is here - https://github.com/tamoyal/wtfrank

Thanks guys! Let me know if I can help

@zzak Take a look at #1236 and let me know your thoughts.

And maybe we should reopen this issue until we decide to close it again with no fix or merge in this patch or another

Closed by #1519. Will be released in v3.0.