error handling not working in "base" Sinatra app

[Alex Chaffee]

The Sinatra Readme says, "The error handler is invoked any time an
exception is raised from a route block or a filter." And this is true
inside a "top level" Sinatra app. But it isn't working in a "base
style" Sinatra app -- I get a blank page in Firefox and in Safari it
says "Safari can’t open the page “http://localhost:4567/” because the
server unexpectedly dropped the connection."

I'll file a ticket and put the code in a gist, but I thought it would
ask if anyone (especially Heroku and/or Exceptional users) have gotten
exception handling to work in production with "base" apps.

http://gist.github.com/292270

https://sinatra.lighthouseapp.com/projects/9779-sinatra/tickets/323-error-block-not-being-called-in-a-sinatrabase-app

---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com

[Ryan Tomayko]

Hi Alex,

This has to do with the different default settings on Sinatra::Base vs
Sinatra::Application (top-level). Specifically, the :raise_errors
option is enabled on Sinatra::Base by default, which causes exceptions
to be raised out of the call method instead of being rescued and put
through the error handlers. The idea is that Base subclasses are
assumed to be part of a bigger app with separate error handling
middleware. Of course, it's fairly common and totally fine for a Base
app to be run standalone, so the defaults often bite people.

You can get the top-level behavior by doing something like:

class Fail < Sinatra::Base
set :raise_errors, false
set :show_exceptions, true if development?
...
end

This has changed somewhat in Sinatra 1.0. Base has :raise_errors
disabled and :show_exceptions enabled in development environments.

Also, there's a table that shows the defaults for Sinatra::Base vs
Sinatra::Default vs. Sinatra::Application here:

https://sinatra.lighthouseapp.com/projects/9779/tickets/312

I'm still not sure we have this just right. I'd love to hear ideas on
how best to determine the defaults for these settings.

Thanks,
Ryan

[Alex Chaffee]

Thanks for the quick reply. My vote would be for the defaults to work
right for the standalone case, but I can see the reasoning for the
other side.

I think what this really points out is (yet again) the need for better
documentation. If the readme had said "see the section
on :raise_errors" it would have pointed me to solving it on my own.
(Not to sound like I'm whining; I know documentation is an ongoing
effort; just giving yet another example of how important it is.)

Sent from my iPhone

> --
> You received this message because you are subscribed to the Google
> Groups "sinatrarb" group.
> To post to this group, send email to sina...@googlegroups.com.
> To unsubscribe from this group, send email to sinatrarb+...@googlegroups.com
> .
> For more options, visit this group at http://groups.google.com/group/sinatrarb?hl=en
> .
>

[Ryan Tomayko]

On Mon, Feb 1, 2010 at 7:09 PM, Alex Chaffee <ale...@gmail.com> wrote:
> Thanks for the quick reply. My vote would be for the defaults to work right
> for the standalone case, but I can see the reasoning for the other side.
>
> I think what this really points out is (yet again) the need for better
> documentation. If the readme had said "see the section on :raise_errors" it
> would have pointed me to solving it on my own. (Not to sound like I'm
> whining; I know documentation is an ongoing effort; just giving yet another
> example of how important it is.)

Most of this is documented on the Options and Configuration page:

http://www.sinatrarb.com/configuration

But I agree, this causes enough confusion that it should probably be
mentioned in the README. Actually, the README doesn't talk about
subclassing Base much at all, led alone how these settings effect it.

Thanks,
Ryan

[Nate W]

I would agree with Alex that the defaults should be geared towards
standalone Sinatra.

If I was mounting the app in Rails, I would expect to have to change
error handling/etc in that case, as part of the setup. But not for
just using Sinatra on its own.

This has bitten me personally repeatedly, since I'll do a prototype as
a classic app, then once it's working, wrap it in a Sinatra::Base
class, then all of a sudden it starts breaking.

-Nate

[Ryan Tomayko]

On Wed, Feb 3, 2010 at 9:22 AM, Nate W <nwi...@gmail.com> wrote:
> I would agree with Alex that the defaults should be geared towards
> standalone Sinatra.
>
> If I was mounting the app in Rails, I would expect to have to change
> error handling/etc in that case, as part of the setup.  But not for
> just using Sinatra on its own.
>
> This has bitten me personally repeatedly, since I'll do a prototype as
> a classic app, then once it's working, wrap it in a Sinatra::Base
> class, then all of a sudden it starts breaking.
>
> -Nate

Yep. I'm fairly certain this is why Sinatra::Default -- which had much
more sane default options for standalone apps -- was used all over the
place in examples and apps in the wild. Default was never really meant
to be part of the exposed API, it was just a technicality in how
reloading was once implemented. But people would subclass Base and
then things like error pages and logging wouldn't work so they
subclassed Default instead and all that stuff worked so they figured
that must be the way to go.

Ryan

> On Feb 1, 8:23 pm, Ryan Tomayko <r...@tomayko.com> wrote:
>> On Mon, Feb 1, 2010 at 7:09 PM, Alex Chaffee <ale...@gmail.com> wrote:
>> > Thanks for the quick reply. My vote would be for the defaults to work right
>> > for the standalone case, but I can see the reasoning for the other side.
>>
>> > I think what this really points out is (yet again) the need for better
>> > documentation. If the readme had said "see the section on :raise_errors" it
>> > would have pointed me to solving it on my own. (Not to sound like I'm
>> > whining; I know documentation is an ongoing effort; just giving yet another
>> > example of how important it is.)
>>
>> Most of this is documented on the Options and Configuration page:
>>
>> http://www.sinatrarb.com/configuration
>>
>> But I agree, this causes enough confusion that it should probably be
>> mentioned in the README. Actually, the README doesn't talk about
>> subclassing Base much at all, led alone how these settings effect it.
>>
>> Thanks,
>> Ryan
>

[Nate W]

So it sounds like we all kinda agree there's an issue here, and given
the matrix and the several tickets about Sinatra::Default in
Lighthouse, I'm guessing it's something that probably bugs you
personally Ryan. :-)

Specifically with error handling, what about just having raise_errors
be false as the default in all cases? That's the only inconsistency/
oddity in this matrix: https://sinatra.lighthouseapp.com/projects/9779/tickets/312

It would make it easier for people to understand the removal
of ::Default as well.

I know 1.0 is getting close, but it seems to me people would expect
potential API/default changes in a major milestone, as opposed to it
being hidden in 1.0.4 or something. Seems like a good time to resolve
this if possible.

-Nate

[Ryan Tomayko]

On Thu, Feb 4, 2010 at 5:05 AM, Nate W <nwi...@gmail.com> wrote:
> So it sounds like we all kinda agree there's an issue here, and given
> the matrix and the several tickets about Sinatra::Default in
> Lighthouse, I'm guessing it's something that probably bugs you
> personally Ryan.  :-)
>
> Specifically with error handling, what about just having raise_errors
> be false as the default in all cases?  That's the only inconsistency/
> oddity in this matrix:  https://sinatra.lighthouseapp.com/projects/9779/tickets/312

We've done that in some cases. Base now has raise_errors disabled and
show_exception enabled under the development environment. I'm not
opposed to making it the default across all environments but I'll have
to stew on it for a bit. I'm definitely interested to hear what people
think of this.

> It would make it easier for people to understand the removal
> of ::Default as well.
>
> I know 1.0 is getting close, but it seems to me people would expect
> potential API/default changes in a major milestone, as opposed to it
> being hidden in 1.0.4 or something.  Seems like a good time to resolve
> this if possible.

Yep. This is precisely the kind of thing I want to get nailed down
before 1.0 is released and why it's taken so long.

Thanks,
Ryan

[Jon]

> Specifically with error handling, what about just having raise_errors
> be false as the default in all cases? That's the only inconsistency/
> oddity in this matrix: https://sinatra.lighthouseapp.com/projects/9779/tickets/312

I'm curious, if the current Sinatra::Base behavior is left as-is for 1.0 but the README is made more clear (including an example and more config info), is your feedback the same?

Jon

[Nate W]

On Feb 4, 5:48 am, Ryan Tomayko <r...@tomayko.com> wrote:
>
> We've done that in some cases. Base now has raise_errors disabled and
> show_exception enabled under the development environment. I'm not
> opposed to making it the default across all environments but I'll have
> to stew on it for a bit. I'm definitely interested to hear what people
> think of this.

Totally understand wanting to think a bit. I'd just like to add that
I think this pre-1.0 solution actually makes things worse. Because
now you'll follow the same workflow I described above - (1) standalone
prototype, (2) wrap in Base, (3) deploy to production, and not until
production will it start breaking, at the worst possible time
(obviously).

And re: Jon's question, it's not a matter of documentation, it's just
unexpected behavior. If you think of a random gem you use, you
wouldn't expect it to act differently regardless of whether you put
the 'require' in a Gemfile or in the actual class that uses it, but
that's essentially what Sinatra::Base does to you in this case.

Sinatra's great, simple, self-explanatory... I would say this is
perhaps the only exception that makes you go "Huh??" It's unexpected
enough that, the first time it hit me, I thought Sinatra::Base itself
was actually buggy.

-Nate

[Ryan Tomayko]

Okay, this should all be a lot more sane now. On master and slated for 1.0:

* Static files are always on by default if the public directory exists.
* Error handlers always override the raise_errors option. If you
register a handler for an exception, it'll be called no matter what.
* The raise_errors option is now disabled by default in all
environments exception for test.

Also, Base and Application behave exactly the same in all of the cases
listed above.

Thanks for everyone's feedback here. This feels a lot less confusing.

Thanks,
Ryan

[Alex Chaffee]

Cool. Thanks, Ryan, it sounds more sane now.

One other use case that came up in my app last week: We wanted the
behavior in development to be different depending on whether the
request was an XHR or not, so our custom JSON error messages make it
back down to the client app, but we can still see the nice "here's the
stack trace and all the variables" page if an uncaught exception
happens in response to an HTML request.

We ended up having to disable all Sinatra and Rack error handling in
all environments (disabling both :show_exceptions and :raise_errors),
then hack in a call to your Sinatra::ShowExceptions object to show the
nice dev error page. It might be an odd case but more and more apps
are using AJAX and JSON so I thought I'd bring it up.

Here's how our error handler looks now (in all its ugly glory, and
including a call out to Exceptional):

error do
e = request.env['sinatra.error']
logger.error "#{e.class}: #{e.message}"
begin
Exceptional::Catcher.handle_with_rack(e, request.env, request)
rescue => exceptional_error
logger.error "#{exceptional_error.class}: #{exceptional_error.message}"
logger.error "\t" + exceptional_error.backtrace.join("\n\t")
end
if request.xhr?
{:message => e.message}.to_json
else
erector {
h1 "Oops"
pre e.message
p "We have been informed of this error. Sorry about that!
We'll get right on it." # todo: real error page

if ENV['RACK_ENV'] == "development"
hr
rawtext Sinatra::ShowExceptions.new(self).pretty(request.env, e)
end

}
end
end

Any constructive criticism?

---
Alex Chaffee - al...@cohuman.com - http://alexch.github.com
Stalk me: http://friendfeed.com/alexch | http://twitter.com/alexch |
http://alexch.tumblr.com

[Nate Wiger]

Ditto - great changes Ryan. Thanks much!

Alex: And I thought our app was the only insane one.

We have the same issue, the only difference is our error handler is
about twice as long.

I'd love to hear any suggestions on how to solve this, really what you
need is per-format error handlers (eg, xml vs html vs json).