Exceptions aren't rescued in a predictable order

[hedgesky]

I'm starting to work on errors handling improvements. First of all, I've introduced a failing spec: from my point of view, rescuing specific error should take precedence over rescuing its base, but it doesn't.
@dblock, could you please explain to me, if it's a bug or a feature? Should I keep this behavior during my changes?

[dblock]

I feel like rescuing errors should follow the rule of least surprise, which is that they should be rescued in order of their declaration until one matches, so RuntimeError first, then ConnectionError in your example. I realize that's not what we have and I would call this a bug.

[hedgesky]

It's look like that there is a little misunderstanding. That new spec fails, and it means that code acts like you expect it to act: the first who matches, wins.
I thought in a bit different way: I expected that the most concrete matching version of rescue_from wins without relying on order. But maybe your approach is more natural.

[hedgesky]

Frankly speaking, I'm quite frustrated after looking on how nested rescues work. The last commit in this PR introduces some specs:

Grape::API
  .mount
    mounting an API
      when some rescues are defined by mounted
        inherits parent rescues
        prefers rescues defined by mounted if they rescue similar error class
        prefers rescues defined by mounted even if outer is more specific (FAILED - 1)
        prefers more specific rescues defined by mounted (FAILED - 2)

The last two ones fail, but I was sure they should be green.
Certainly, I can inspect source and say, why it behaves like this, but I'm more curious about how it is intended to behave.
Thanks for your answers and time spent on them!

[dblock]

There's ordering of rescues and there's mounting of other APIs. I'll maintain that what we always want is to rescue in order. That's partly because route/api matching works like this, imagine if a more specific route was matched first, it would make things very hard to predict and lots of side effects.

Why is the above a problem? Why can't one order rescues the way you want them to match? Why is that frustrating?

[hedgesky]

You know, I thought they are something like overriding a method in a class descendants: if both parent and child have a method :do_smth, child's one takes a priority. But I see your point now, thanks for clarifying.
But why then spec prefers rescues defined by mounted if they rescue similar error class doesn't fail?

[dblock]

Not sure. I cannot find that spec - want to copy the relevant code out here or give a link?

[hedgesky]

Here is the link. Or you could just paste this code to api_spec.rb:

it 'prefers rescues defined by mounted if they rescue similar error class' do
  subject.rescue_from StandardError do
    rack_response("outer rescue")
  end

  app = Class.new(Grape::API)

  subject.namespace :mounted do
    rescue_from StandardError do
      rack_response("inner rescue")
    end
    app.get('/fail') { raise 'doh!' }
    mount app
  end

  get '/mounted/fail'
  expect(last_response.body).to eq('inner rescue')
end

If I understand you correctly, it should fail (returning 'outer rescue'). But it's green.

[dblock]

Not here because of namespace. Namespace is a container and in this case the order of evaluation is within the container first, then outside of the container. I think it's logical, because when you develop inside of the container (could be a standalone API or a namespaced API) you want to have full control over what's going on.

I guess the metaphor of a list isn't really what we have here. We have a tree. Routes evaluate sequentially going down the tree by doing a depth-first-search, exceptions bubble up the other way around, exactly like any recursive program.

[hedgesky]

But then these specs should be green, too. They also use namespace, but with little changes in rescue_from clauses.

it 'prefers rescues defined by mounted even if outer is more specific' do
  subject.rescue_from ArgumentError do
    rack_response("outer rescue")
  end

  app = Class.new(Grape::API)

  subject.namespace :mounted do
    rescue_from StandardError do
      rack_response("inner rescue")
    end
    app.get('/fail') { raise ArgumentError.new }
    mount app
  end

  get '/mounted/fail'
  expect(last_response.body).to eq('inner rescue')
end
it 'prefers more specific rescues defined by mounted' do
  subject.rescue_from StandardError do
    rack_response("outer rescue")
  end

  app = Class.new(Grape::API)

  subject.namespace :mounted do
    rescue_from ArgumentError do
      rack_response("inner rescue")
    end
    app.get('/fail') { raise ArgumentError.new }
    mount app
  end

  get '/mounted/fail'
  expect(last_response.body).to eq('inner rescue')
end

That's why I was frustrated: in different situations different priority is observed.

[dblock]

I agree, both should succeed here. Can you fix the rubocop stuff in this PR please so we can all see the spec failures?

[hedgesky]

I've updated the branch. These specs fails:

./spec/grape/api_spec.rb:2693
./spec/grape/api_spec.rb:2711

[hedgesky]

I think the issue appears because here we use stackable setting. It gives us an ordered collection (hash in this case) where inherited values are in the beginning: link.
Later, when we actually look for a handler, we take first suitable element. And it will be outer rescue, which is wrong.
Honestly, I don't know how to solve the issue without a big effort. As for me, neither InheritableValues nor StackableValues don't fit the situation. Maybe smth like TreeValues is needed, with which we can traverse a tree from leaves up to root, searching for a first matching element?

[dblock]

I think you have it exactly right @hedgesky. Marking this as confirmed bug.

[hedgesky]

I've added a new class called ReverseStackableValues. It's similar to StackableValues, but the priority of inherited/new values is changed (new values first). Maybe the name isn't perfect, but I couldn't come up with a better one.
I've used the class to storing rescue_from handlers, and it has solved the problem.
Also, I couldn't find out any usages of StackableValues#freeze_value. Is it needed?

[dblock]

This PR is great, well done @hedgesky, lets get this merged.

Can you please add a line to CHANGELOG and write a paragraph in UPGRADING for the scenarios that will break? Probably want some more README updates too. Squash your commits, too.

If a method is unused it can be deleted ;)

[hedgesky]

If a method is unused it can be deleted ;)

Not that simple: there are InheritableValues#initialize_copy and StackableValues#initialize_copy methods which are not used in the project directly. But if you try to remove them specs will start to fail. So, they are used somewhere internally. So I wonder if #freeze_value was introduced just in case or in order to support some underlying functionality (marshalling or smth like that).
However, I'll remove them in separate PR to distinguish changes in commits.

Regarding changelog, readme and upgrading — will do that soon.

[hedgesky]

Is everything OK with it?

[dblock]

Yep, Github doesn't send a notification on a push. Thank you, merging!