Callbacks: leave less lines in the backtrace #26147

Merged
merged 1 commit into from Sep 30, 2016

Conversation

Projects
None yet
4 participants
@matthewd
Member

matthewd commented Aug 13, 2016

Callbacks are everywhere, so it's better if we can avoid making a mess of the backtrace just because we've passed through a callback hook.

Before:

RuntimeError: inside save
    test/callbacks_test.rb:73:in `block in save'
    lib/active_support/callbacks.rb:126:in `call'
    lib/active_support/callbacks.rb:506:in `block (2 levels) in compile'
    lib/active_support/callbacks.rb:455:in `call'
    lib/active_support/callbacks.rb:101:in `__run_callbacks__'
    lib/active_support/callbacks.rb:755:in `_run_save_callbacks'
    lib/active_support/callbacks.rb:90:in `run_callbacks'
    test/callbacks_test.rb:72:in `save'
    test/callbacks_test.rb:526:in `test_save_person'

After:

RuntimeError: inside save
    test/callbacks_test.rb:73:in `block in save'
    lib/active_support/callbacks.rb:133:in `run_callbacks'
    test/callbacks_test.rb:72:in `save'
    test/callbacks_test.rb:526:in `test_save_person'

And a more extreme example, involving around callbacks:

Before:

RuntimeError: inside save
    test/callbacks_test.rb:293:in `block in save'
    lib/active_support/callbacks.rb:126:in `call'
    lib/active_support/callbacks.rb:506:in `block (2 levels) in compile'
    lib/active_support/callbacks.rb:455:in `call'
    lib/active_support/callbacks.rb:448:in `block (2 levels) in around'
    lib/active_support/callbacks.rb:286:in `block (2 levels) in halting'
    test/callbacks_test.rb:283:in `tweedle_deedle'
    lib/active_support/callbacks.rb:382:in `block in make_lambda'
    lib/active_support/callbacks.rb:285:in `block in halting'
    lib/active_support/callbacks.rb:447:in `block in around'
    lib/active_support/callbacks.rb:455:in `call'
    lib/active_support/callbacks.rb:448:in `block (2 levels) in around'
    lib/active_support/callbacks.rb:271:in `block in halting_and_conditional'
    lib/active_support/callbacks.rb:447:in `block in around'
    lib/active_support/callbacks.rb:455:in `call'
    lib/active_support/callbacks.rb:448:in `block (2 levels) in around'
    lib/active_support/callbacks.rb:267:in `block (2 levels) in halting_and_conditional'
    test/callbacks_test.rb:258:in `w0tyes'
    lib/active_support/callbacks.rb:382:in `block in make_lambda'
    lib/active_support/callbacks.rb:266:in `block in halting_and_conditional'
    lib/active_support/callbacks.rb:447:in `block in around'
    lib/active_support/callbacks.rb:455:in `call'
    lib/active_support/callbacks.rb:448:in `block (2 levels) in around'
    lib/active_support/callbacks.rb:286:in `block (2 levels) in halting'
    test/callbacks_test.rb:273:in `tweedle_dum'
    lib/active_support/callbacks.rb:382:in `block in make_lambda'
    lib/active_support/callbacks.rb:285:in `block in halting'
    lib/active_support/callbacks.rb:447:in `block in around'
    lib/active_support/callbacks.rb:455:in `call'
    lib/active_support/callbacks.rb:101:in `__run_callbacks__'
    lib/active_support/callbacks.rb:755:in `_run_save_callbacks'
    lib/active_support/callbacks.rb:90:in `run_callbacks'
    test/callbacks_test.rb:292:in `save'
    test/callbacks_test.rb:395:in `test_save_around'

After:

RuntimeError: inside save
    test/callbacks_test.rb:293:in `block in save'
    lib/active_support/callbacks.rb:116:in `block in run_callbacks'
    test/callbacks_test.rb:283:in `tweedle_deedle'
    lib/active_support/callbacks.rb:122:in `block in run_callbacks'
    test/callbacks_test.rb:258:in `w0tyes'
    lib/active_support/callbacks.rb:122:in `block in run_callbacks'
    test/callbacks_test.rb:273:in `tweedle_dum'
    lib/active_support/callbacks.rb:122:in `block in run_callbacks'
    lib/active_support/callbacks.rb:137:in `run_callbacks'
    test/callbacks_test.rb:292:in `save'
    test/callbacks_test.rb:395:in `test_save_around'
- runner = callbacks.compile
- e = Filters::Environment.new(self, false, nil, block)
- runner.call(e).value
+ invoke_sequence.call

This comment has been minimized.

@matthewd

matthewd Aug 13, 2016

Member

In-lining a copy of invoke_sequence here would eliminate the remaining double entry when an around is defined:

    lib/active_support/callbacks.rb:122:in `block in run_callbacks'
    lib/active_support/callbacks.rb:137:in `run_callbacks'

But that doesn't seem like a sensible trade.

(I actually like the symmetry with it there, anyway.)

@matthewd

matthewd Aug 13, 2016

Member

In-lining a copy of invoke_sequence here would eliminate the remaining double entry when an around is defined:

    lib/active_support/callbacks.rb:122:in `block in run_callbacks'
    lib/active_support/callbacks.rb:137:in `run_callbacks'

But that doesn't seem like a sensible trade.

(I actually like the symmetry with it there, anyway.)

+
+ result.unshift @method_name
+ result.unshift @override_block || block
+ result.unshift @override_target || target

This comment has been minimized.

@matthewd

matthewd Aug 13, 2016

Member

This awkward construction of the array keeps us down to one allocation, in the map. The callers likewise shift the values back out, for the same reason.

@matthewd

matthewd Aug 13, 2016

Member

This awkward construction of the array keeps us down to one allocation, in the map. The callers likewise shift the values back out, for the same reason.

@@ -747,12 +803,25 @@ def define_callbacks(*names)
options = names.extract_options!
names.each do |name|
- class_attribute "_#{name}_callbacks", instance_writer: false
+ name = name.to_sym
+
set_callbacks name, CallbackChain.new(name, options)
module_eval <<-RUBY, __FILE__, __LINE__ + 1

This comment has been minimized.

@matthewd

matthewd Aug 13, 2016

Member

Do we need to keep any/all of these? If so, should they be deprecated?

@matthewd

matthewd Aug 13, 2016

Member

Do we need to keep any/all of these? If so, should they be deprecated?

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 16, 2016

Member

I don't think we need to. They should not be public API, so if we don't need inside the framework we can remove without deprecation.

@rafaelfranca

rafaelfranca Aug 16, 2016

Member

I don't think we need to. They should not be public API, so if we don't need inside the framework we can remove without deprecation.

This comment has been minimized.

@matthewd

matthewd Aug 16, 2016

Member

There is one mention, but maybe it's up to AR to provide and deprecate those.

@matthewd

matthewd Aug 16, 2016

Member

There is one mention, but maybe it's up to AR to provide and deprecate those.

+ "block in run_callbacks",
+ "tweedle_dum",
+ "block in run_callbacks",
+ ("call" if RUBY_VERSION < "2.3"),

This comment has been minimized.

@matthewd

matthewd Aug 13, 2016

Member

😢

activesupport/test/callbacks_test.rb
+ exception = (around.save rescue $!)
+
+ # Make sure we have the exception we're expecting
+ assert "inside save", exception.message

This comment has been minimized.

@kaspth

kaspth Aug 13, 2016

Member

This needs to be assert_equal.

@kaspth

kaspth Aug 13, 2016

Member

This needs to be assert_equal.

This comment has been minimized.

@matthewd

matthewd Aug 13, 2016

Member

😳

activesupport/test/callbacks_test.rb
+ exception = (person.save rescue $!)
+
+ # Make sure we have the exception we're expecting
+ assert "inside save", exception.message

This comment has been minimized.

@kaspth

kaspth Aug 13, 2016

Member

Same here.

@kaspth

kaspth Aug 13, 2016

Member

Same here.

+
+ invoke_sequence = Proc.new do
+ skipped = nil
+ while true

This comment has been minimized.

@kaspth

kaspth Aug 13, 2016

Member

loop do?

@kaspth

kaspth Aug 13, 2016

Member

loop do?

This comment has been minimized.

@matthewd

matthewd Aug 13, 2016

Member

Method call = stack frame 😉

@matthewd

matthewd Aug 13, 2016

Member

Method call = stack frame 😉

This comment has been minimized.

@kaspth

kaspth Aug 13, 2016

Member

Ha, yeah, then that would kinda work against the idea 😄

@kaspth

kaspth Aug 13, 2016

Member

Ha, yeah, then that would kinda work against the idea 😄

+ expanded.shift.send(*expanded, &expanded.shift)
+ end
+ current.invoke_after(env)
+ skipped.pop.invoke_after(env) while skipped && skipped.first

This comment has been minimized.

@kaspth

kaspth Aug 13, 2016

Member

Why do we run the after callbacks to code that we've skipped?

@kaspth

kaspth Aug 13, 2016

Member

Why do we run the after callbacks to code that we've skipped?

This comment has been minimized.

@matthewd

matthewd Aug 13, 2016

Member

invoke_before / invoke_after handle before and after callbacks completely internally, including their conditionals, etc. Here, the skipping refers exclusively to the around callback which is the centrepiece of the CallbackSequence.

@matthewd

matthewd Aug 13, 2016

Member

invoke_before / invoke_after handle before and after callbacks completely internally, including their conditionals, etc. Here, the skipping refers exclusively to the around callback which is the centrepiece of the CallbackSequence.

This comment has been minimized.

@kaspth

kaspth Aug 13, 2016

Member

Ah, so if we're halted any after callbacks won't be run, right?

I still don't understand why we need to keep track of skipped. I can't see the around callbackness of it.

@kaspth

kaspth Aug 13, 2016

Member

Ah, so if we're halted any after callbacks won't be run, right?

I still don't understand why we need to keep track of skipped. I can't see the around callbackness of it.

This comment has been minimized.

@matthewd

matthewd Aug 13, 2016

Member
around_save :foo, if: :no
after_save :bar

Both of the above live in the same CallbackSequence. It will be skipped because the condition on foo is false... but we still have to run bar. (Unless we're halted.. but invoke_after takes care of that.)

If it was not skipped, then we'd call foo and ultimately recurse back into a fresh invocation of invoke_sequence, and bar would get run by that invocation on its way out, as current.

@matthewd

matthewd Aug 13, 2016

Member
around_save :foo, if: :no
after_save :bar

Both of the above live in the same CallbackSequence. It will be skipped because the condition on foo is false... but we still have to run bar. (Unless we're halted.. but invoke_after takes care of that.)

If it was not skipped, then we'd call foo and ultimately recurse back into a fresh invocation of invoke_sequence, and bar would get run by that invocation on its way out, as current.

This comment has been minimized.

@kaspth

kaspth Aug 14, 2016

Member

Ah, right. So we accumulate the skipped callbacks and make sure to call invoke_after on them which will then handle running something if need be. Or just execute them via current. Makes sense.

Does running current.invoke_after and then skipped.pop..invoke_after alter the after callback run order from the previous implementation? Or is that just fine?

@kaspth

kaspth Aug 14, 2016

Member

Ah, right. So we accumulate the skipped callbacks and make sure to call invoke_after on them which will then handle running something if need be. Or just execute them via current. Makes sense.

Does running current.invoke_after and then skipped.pop..invoke_after alter the after callback run order from the previous implementation? Or is that just fine?

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 13, 2016

Member

Dig the change 👍

Member

kaspth commented Aug 13, 2016

Dig the change 👍

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Aug 17, 2016

Member

Have you checked if this will fix #25068?

Member

rafaelfranca commented Aug 17, 2016

Have you checked if this will fix #25068?

@eugeneius

This comment has been minimized.

Show comment
Hide comment
@eugeneius

eugeneius Aug 19, 2016

Member

I re-ran the example app from #25068 against both master and this branch, and the behaviour was the same as before in both cases.

Member

eugeneius commented Aug 19, 2016

I re-ran the example app from #25068 against both master and this branch, and the behaviour was the same as before in both cases.

Tighten the backtrace pollution from passing through callbacks
Callbacks are everywhere, so it's better if we can avoid making a mess
of the backtrace just because we've passed through a callback hook.

I'm making no effort to the before/after invocations: those only affect
backtraces while they're running. The calls that matter are the ones
that remain on the call stack after run_callbacks yields: around
callbacks, and internal book-keeping around the before/afters.

@matthewd matthewd merged commit 4f2d1d6 into rails:master Sep 30, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

amatsuda added a commit to asakusarb/action_args that referenced this pull request Nov 3, 2016

@matthewd matthewd deleted the matthewd:callback-backtrace branch Jun 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment