Fix raise and throws in instrument helper

[tombruijn]

By using a "soft return", by saving the return value in a variable, it
can happen that an event is not finished when the block that's given to
an instrument method raises and error or uses throw to throw a signal.
Using ensure we ensure that the event is finished even when that
happens.

Reported in

As reported in #289 (comment). Doesn't fix the (potential) parent issue of padrino starting two transactions in certain scenarios, still debugging that.

Notes

• Separated from PR Remove duplicate implementation of .instrument #288 which highlights a too tightly coupled spec with mocks in the Padrino specs.
• Aimed at a 2.2.1 release on the master branch

[thijsc]

I think this was necessary to actually return the proper value in all scenarios. Doesn't this break some tests?

[tombruijn]

It shouldn't, but let's wait on TravisCI to give the verdict in combination for all gems.

The use of ensure, rather than using a return_value variable, doesn't affect the return value of a block. It allows you to execute code whatever happens beforehand, in this case a raise or a throw.

def foo
  "bar"
ensure
  123
end

foo # => "bar"

returns "bar" as a return value from the method.

See also this spec: https://github.com/appsignal/appsignal-ruby/pull/292/files#diff-0968d4ed9ff8f5b249a2c7e8bc32784bR19

[thijsc]

Ok. If it works that's of course fine. It's just that I remember adding this for a specific scenario that was broken, but not exactly sure what it was.

[thijsc]

Ah I get it. The current implementation is:

    def instrument(name, title = nil, body = nil, body_format = Appsignal::EventFormatter::DEFAULT)
      Appsignal::Transaction.current.start_event
      return_value = yield if block_given?
      Appsignal::Transaction.current.finish_event(name, title, body, body_format)
      return_value
    end

So the new implementation that uses ensure is just a better way of making this behaviour happen.