Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce explicit way of halting callback chains by throwing :abort. Deprecate current implicit behavior of halting callback chains by returning `false` in apps ported to Rails 5.0. Completely remove that behavior in brand new Rails 5.0 apps. #17227

Merged
merged 7 commits into from Jan 3, 2015

Conversation

@claudiob
Copy link
Member

@claudiob claudiob commented Oct 10, 2014

Update (2015-01-02): a gist with the suggested release notes to add to Rails 5.0 after this commit is available at https://gist.github.com/claudiob/614c59409fb7d11f2931


Stems from discussion with @dhh at https://groups.google.com/forum/#!topic/rubyonrails-core/mhD4T90g0G4

@dhh – I created this work-in-progress PR to continue the conversation with some code.
Could you tell me if this is what you intended by using :throw with a symbol?

I have quite clear how to add the throw to the code in ActiveJob, but not as
much where to catch it in ActiveSupport. For now I have this code:

def run_callbacks(kind, &block)
  catch(:abort_job)  do
    send "run_#{kind}_callbacks", &block
  end
end

but maybe the catch is better located somewhere at a deeper level of the callback stack.

@dhh Thoughts? Once I have a clearer idea on how to proceed, I can complete
this PR with tests, documentations, etc. Thanks!

@dhh
Copy link
Member

@dhh dhh commented Oct 16, 2014

Looking pretty good. I'd actually like to bubble this all the way up to ActiveSupport, though. IMO, it should be a generic feature of the callback code to have "throw :abort" that'll cancel the entire chain. Thanks for working on this!

@claudiob claudiob force-pushed the claudiob:explicitly-abort-callbacks branch Oct 16, 2014
@claudiob claudiob changed the title Throw :abort_job to exit a callback chain Throw :halt_callbacks halts default CallbackChains Oct 16, 2014
@claudiob
Copy link
Member Author

@claudiob claudiob commented Oct 16, 2014

@dhh I investigated a little deeper and found that I did not have to add a brand new method to halt the execution of a chain callback: there is already a :terminator option that is meant for that:

:terminator - Determines when a before filter will halt the callback chain, preventing following callbacks from being called and the event from being triggered. This should be a lambda to be executed. Defaults to false, meaning no value halts the chain.

So my suggestion is: let's change the default value of :terminator, from false (never halt the chain) to a lambda that halts the chain if the callback includes throw(:halt_callbacks).

The advantage of this method is that it is backward-compatible for the modules that explicitly define a :terminator. For instance, ActiveModel currently define callbacks with the following terminator:

terminator: ->(_,result) { result == false },

meaning that any before_ callback returning false halts the chain.

This PR does not change that behavior, but prepares the way for the change you suggested on the Google Group. When the day comes, we can simply remove the terminator above, and have ActiveModel use the default terminator introduced by this PR, which ignores false values and only halts the chain when :halt_callbacks is thrown. (update: this PR now deprecates that behavior)

As you read the list of files changed by this PR, keep in mind that activesupport/lib/active_support/callbacks.rb is the key one; however, I also had the change the files where terminators are defined, since they now have to accept a Proc, rather than a value, as the second parameter.

In this way, I am able catch(:halt_callbacks) only in one place of the code, by invoking the passed Proc. Otherwise, I would have to catch the message in every single define_callback.

Please tell me what you think, and also if you think :halt_callbacks is a good name for the message.
Thanks! 🍭

@dhh
Copy link
Member

@dhh dhh commented Oct 16, 2014

This seems better, but I don't like :halt_callbacks, because we're not just halting the callbacks, we're halting the whole chain which includes both the callbacks AND the actual action we're wrapping. The shorter throw :abort just strikes me as more pleasing too. But otherwise looks good to me. //cc @rafaelfranca @tenderlove

@claudiob claudiob force-pushed the claudiob:explicitly-abort-callbacks branch Oct 16, 2014
@claudiob
Copy link
Member Author

@claudiob claudiob commented Oct 17, 2014

@dhh I'm fine with any name.

I have an additional note.
Until now, only before_ callbacks are able to halt the chain; the return value of after_ callbacks is ignored. If after_ callbacks are executed, then they are all executed.

With this PR, the behavior does not change. Therefore, if an after_ callback decided to throw(:abort), this would not be caught by terminator: it would simply bubble up to the user.

I think this okay, since it matches what the documentation says and existing code.
I just wanted to confirm.

@dhh
Copy link
Member

@dhh dhh commented Oct 17, 2014

Ultimately, I think you should be able to halt the after_* chain as well. But we don’t need to deal with it in the same PR.

On Oct 16, 2014, at 17:02, Claudio B. notifications@github.com wrote:

@dhh I'm fine with any name.

I have an additional note.
Until now, only before_ callbacks are able to halt the chain; the return value of after_ callbacks is ignored. If after_ callbacks are executed, then they are all executed.

With this PR, the behavior does not change. Therefore, if an after_ callback decided to throw(:abort), then it wouldn't be cached by terminator, and it would simply bubble up to the user.

I think this okay, since it matches what the documentation says and existing code.
I just wanted to confirm.


Reply to this email directly or view it on GitHub.

@tenderlove
Copy link
Member

@tenderlove tenderlove commented Oct 17, 2014

Any reason to use throw / catch vs an exception? We should probably test the performance impact of this change as well. Callbacks are a huge hotspot (since we use them literally everywhere).

@dhh
Copy link
Member

@dhh dhh commented Oct 17, 2014

Exceptions should only be when something is going wrong, really. We need a way to control the flow when aborting the callbacks is the expected path.

On Oct 16, 2014, at 18:55, Aaron Patterson notifications@github.com wrote:

Any reason to use throw / catch vs an exception? We should probably test the performance impact of this change as well. Callbacks are a huge hotspot (since we use them literally everywhere).


Reply to this email directly or view it on GitHub.

@claudiob claudiob force-pushed the claudiob:explicitly-abort-callbacks branch Oct 17, 2014
@claudiob claudiob changed the title Throw :halt_callbacks halts default CallbackChains Throw :abort halts default CallbackChains Oct 17, 2014
@claudiob claudiob force-pushed the claudiob:explicitly-abort-callbacks branch Oct 17, 2014
@egilburg
Copy link
Contributor

@egilburg egilburg commented Oct 17, 2014

Perhaps it's wrong assumption to make but I'd assume seeing abort in a save callback would cancel/reverse the save, even if it's an after_save callback (thinking about the "abort transaction" concept). For this reason :halt seemed better to me.

Perhaps break or return, similar to Ruby's syntax for loops and method respectively? Or a more explicit halt_callbacks or stop_callbacks, with plural "callbacks" implying all further ones will be stopped and not just current one?

@dhh
Copy link
Member

@dhh dhh commented Oct 18, 2014

Halting execution during the before callbacks is the primary use case, and stopping not just the callbacks, but the action itself, from being executed is the primary within that. So given that setup, I still prefer something that doesn't have *_callbacks. "throw halt" sounds a little weird to me. "throw abort" seems nicer.

"throw abort" will cancel/reverse the save if its still under the same transaction, so that seems accurate too.

@claudiob claudiob force-pushed the claudiob:explicitly-abort-callbacks branch Dec 1, 2014
@claudiob
Copy link
Member Author

@claudiob claudiob commented Dec 1, 2014

Hello @dhh @pixeltrix @sferik @wjessop @tomstuart

Now that Rails 4.2.rc1 is out, it's probably a good time to continue the discussion on this ticket, which also matches what you guys discussed on Twitter.

The recap of this PR is: change the behavior of callbacks so that returning false in a before_ callback does not halt the chain anymore. The callback chain can only be halted explicitly by calling throw :abort.

More details in the comments above! 🍭

@dhh
Copy link
Member

@dhh dhh commented Dec 5, 2014

👍

@claudiob claudiob force-pushed the claudiob:explicitly-abort-callbacks branch Dec 5, 2014
@claudiob
Copy link
Member Author

@claudiob claudiob commented Dec 5, 2014

@dhh Rebased and conflicts solved!

@dhh
Copy link
Member

@dhh dhh commented Dec 5, 2014

Looks good to me, but I'll let @rafaelfranca or someone else on the team have a look as well.

@matthewd
Copy link
Member

@matthewd matthewd commented Dec 7, 2014

I've only skimmed, but it sounds like we're missing a deprecation here at the moment?

I would expect that a falsey result would emit a deprecation warning, but still halt the chain, for a version.

@dhh
Copy link
Member

@dhh dhh commented Dec 7, 2014

Let's get that into 4.2.

On Dec 7, 2014, at 4:56 PM, Matthew Draper notifications@github.com wrote:

I've only skimmed, but it sounds like we're missing a deprecation here at the moment?

I would expect that a falsey result would emit a deprecation warning, but still halt the chain, for a version.


Reply to this email directly or view it on GitHub.

@matthewd
Copy link
Member

@matthewd matthewd commented Dec 7, 2014

@dhh the only way to get the deprecation into 4.2 would be to ship this whole feature change in 4.2. And it's way too late for that -- especially considering @tenderlove's very reasonable performance concerns.

With a deprecation warning, we can ship this as the right way to do things in 5.0... it's just that the old way will still work (noisily) until 5.1.

@dhh
Copy link
Member

@dhh dhh commented Dec 7, 2014

Fine be me too.

On Dec 7, 2014, at 5:39 PM, Matthew Draper notifications@github.com wrote:

@dhh the only way to get the deprecation into 4.2 would be to ship this whole feature change in 4.2. And it's way too late for that -- especially considering @tenderlove's very reasonable performance concerns.

With a deprecation warning, we can ship this as the right way to do things in 5.0... it's just that the old way will still work (noisily) until 5.1.


Reply to this email directly or view it on GitHub.

@claudiob
Copy link
Member Author

@claudiob claudiob commented Dec 7, 2014

@matthewd @dhh I'm not 100% sure this PR needs a deprecation policy.

Let me explain: the behavior of callback chains that define a terminator does not change after this PR.
For instance, ActiveModel's callback chains will still halt if any before_ callback returns false.

This PR adds a new behavior for callback chains that do not define a terminator.

Before this PR, if your callback chain did not define a terminator, then the callback chain could never halted.

After this PR, if your callback chain does not define a terminator, then you have the possibility to halt the callback chain by throwing :abort in a before_ callback.

I think a deprecation policy will only be needed once we change existing behaviors: for instance, if we decide that returning false in an ActiveModel before_ callback won't halt the chain anymore, or if we decide that callback chains can also be halted by an after_ callback. None of that is part of this PR.

Let me know what you think!

@dhh
Copy link
Member

@dhh dhh commented Dec 8, 2014

Let's expand this PR to include the deprecation. The main benefit of this switch is not using throw :abort when you know you want to halt, but to avoid halting when the callback accidentally returns false.

On Dec 7, 2014, at 22:31, Claudio B. notifications@github.com wrote:

@matthewd @dhh I'm not 100% sure this PR needs a deprecation policy.

Let me explain: the behavior of callback chains that define a terminator does not change after this PR.
For instance, ActiveModel's callback chains will still halt if any before_ callback returns false.

This PR adds a new behavior for callback chains that do not define a terminator.

Before this PR, if your callback chain did not define a terminator, then the callback chain could never halted.

After this PR, if your callback chain does not define a terminator, then you have the possibility to halt the callback chain by throwing :abort in a before_ callback.

I think a deprecation policy will only be needed once we change existing behaviors: for instance, if we decide that returning false in an ActiveModel before_ callback won't halt the chain anymore, or if we decide that callback chains can also be halted by an after_ callback. None of that is part of this PR.

Let me know what you think!


Reply to this email directly or view it on GitHub.

@claudiob claudiob force-pushed the claudiob:explicitly-abort-callbacks branch Dec 8, 2014
@claudiob
Copy link
Member Author

@claudiob claudiob commented Dec 8, 2014

@dhh I have followed your suggestion and included the deprecation in the three places where callback chains can be halted by returning false:

  1. ActiveModel Validation callbacks
  2. ActiveModel callbacks
  3. ActiveRecord callbacks

As a result of this PR, returning false in a before_ callback will still work as before, but will display the following message:

DEPRECATION WARNING: Returning false in a before_ callback will not implicitly halt a callback chain in the next release of Rails. To explicitly halt a callback chain, please use throw :abort instead.

Please tell me if you'd like to rephrase the message. The message should make clear that returning false in a callback is not wrong but in future version of Rails will not have the implicit side effect it has now.

@dhh
Copy link
Member

@dhh dhh commented Dec 8, 2014

Looks good to me 👍

On Dec 8, 2014, at 4:23 PM, Claudio B. notifications@github.com wrote:

@dhh I have followed your suggestion and included the deprecation in the three places where callback chains can be halted by returning false:

• ActiveModel Validation callbacks
• ActiveModel callbacks
• ActiveRecord callbacks
As a result of this PR, returning false in a before_ callback will still work as before, but will display the following message:

DEPRECATION WARNING: Returning false in a before_ callback will not implicitly halt a callback chain in the next release of Rails. To explicitly halt a callback chain, please use throw :abort instead.

Please tell me if you'd like to rephrase the message. The message should make clear that returning false in a callback is not wrong but in future version of Rails will not have the implicit side effect it has now.


Reply to this email directly or view it on GitHub.

@claudiob
Copy link
Member Author

@claudiob claudiob commented Jun 19, 2015

==> #20612

claudiob added a commit to claudiob/rails that referenced this pull request Sep 13, 2015
Fixes rails#21122 - does not change any current behavior; simply reflects
the fact that two conditions of the if/else statement are never reached.

The reason is rails#17227 which adds a default terminator to AS::Callbacks.

Therefore, even callback chains that do not define a terminator now
have a terminator, and `chain_config.key?(:terminator)` is always true.

Of course, if no terminator was defined, then we want this new default
terminator not to do anything special. What the terminator actually does
(or should do) is discussed in rails#21218 but the simple fact that a default
terminator exists makes this current PR valid.
claudiob added a commit to claudiob/rails that referenced this pull request Sep 13, 2015
Fixes rails#21122 - does not change any current behavior; simply reflects
the fact that two conditions of the if/else statement are never reached.

The reason is rails#17227 which adds a default terminator to AS::Callbacks.

Therefore, even callback chains that do not define a terminator now
have a terminator, and `chain_config.key?(:terminator)` is always true.

Of course, if no terminator was defined, then we want this new default
terminator not to do anything special. What the terminator actually does
(or should do) is discussed in rails#21218 but the simple fact that a default
terminator exists makes this current PR valid.
claudiob added a commit to claudiob/rails that referenced this pull request Sep 13, 2015
Fixes rails#21122 - does not change any current behavior; simply reflects
the fact that two conditions of the if/else statement are never reached.

The reason is rails#17227 which adds a default terminator to AS::Callbacks.

Therefore, even callback chains that do not define a terminator now
have a terminator, and `chain_config.key?(:terminator)` is always true.

Of course, if no terminator was defined, then we want this new default
terminator not to do anything special. What the terminator actually does
(or should do) is discussed in rails#21218 but the simple fact that a default
terminator exists makes this current PR valid.
claudiob added a commit to claudiob/rails that referenced this pull request Sep 13, 2015
Fixes rails#21122 - does not change any current behavior; simply reflects
the fact that two conditions of the if/else statement are never reached.

The reason is rails#17227 which adds a default terminator to AS::Callbacks.

Therefore, even callback chains that do not define a terminator now
have a terminator, and `chain_config.key?(:terminator)` is always true.

Of course, if no terminator was defined, then we want this new default
terminator not to do anything special. What the terminator actually does
(or should do) is discussed in rails#21218 but the simple fact that a default
terminator exists makes this current PR valid.

*Note* that the conditional/simple methods have not been removed in
AS::Conditionals::Filter::After because of `:skip_after_callbacks_if_terminated`
which lets a user decide **not** to skip after callbacks even if the chain was
terminated.
betesh added a commit to betesh/paperclip that referenced this pull request Jan 20, 2016
rails/rails#17227 introduces a change to the way execution is halted by a before callback.  Instead of the before-hook returning false, it must `throw(:abort)`.

This feature is opt-in for projects upgrading from older versions of Rails, so we need to handle both new and upgrading apps by checking whether they are opted into this new behavior yet.
@dnsco
Copy link

@dnsco dnsco commented Mar 8, 2016

Is there anyway the scope of this could be increased to allow you to throw in controller methods that are run as guard clauses rather than before_filters to abort the rest of the action.

It would be really nice for avoiding DoubleRenderErrors.

Trivial Case:

class ThingsController < ApplicationController
  def index
    ensure_authorized
  end

  protected

  def ensure_authorized
    redirect_to root_url
    throw(:abort)
  end
end

I will gladly submit a pull request if it would be accepted, but don't want to waste time.

/cc @claudiob @dhh

@claudiob
Copy link
Member Author

@claudiob claudiob commented Mar 9, 2016

@denniscollective I think what you suggest would be overwhelming, meaning that we would have to wrap every action inside a catch(:abort), and we would let every method (not just "guard clauses") throw(:abort) and expect the action to be interrupted.

In the example above, what would you like to happen if the user is not authorized?
If you have something in mind, you can explicitly write it as:

def index
  if authorized
    # .. render view or any behavior that you desire
  else
    # .. redirect_to with a flash or any behavior that you desire
  end
end

If you really want the behavior provided by throw(:abort), you can instead move ensure_authorized from inside the index method into before_action :ensure_authorized, only: :index.

jcoyne added a commit to samvera/active_fedora that referenced this pull request Jul 29, 2016
cbeer added a commit to samvera-labs/active_fedora-datastreams that referenced this pull request Aug 18, 2016
cbeer added a commit to samvera-labs/active_fedora-datastreams that referenced this pull request Aug 18, 2016
@seanhandley seanhandley mentioned this pull request Dec 17, 2016
23 of 23 tasks complete
@yamanaltereh
Copy link

@yamanaltereh yamanaltereh commented Mar 1, 2017

@claudiob I used throw(:abort), but for save! it should return false not raise exception!

@claudiob
Copy link
Member Author

@claudiob claudiob commented Mar 1, 2017

@yamanaltereh Hello! Can you provide a little more context or a snippet of code?

If you found a bug, a bug report would also be very useful. Thanks!

toru added a commit to toru/atlas that referenced this pull request May 16, 2017
carlosantoniodasilva added a commit to heartcombo/mail_form that referenced this pull request Aug 28, 2020
To integrate with Active Record you just need to
`include MailForm::Delivery` and implement the `headers` method. An
email should be sent whenever the model is created successfully.

That was broken since we introduced these extra conditionals to fix the
delivery of MailForm, due to the change in how Rails handles callbacks
abortion to not rely on the `false` value returning from `before_*`
callbacks, to explicitly require the code to `throw(:abort)` instead.

This was happening for objects inheriting from `MailForm::Base` so it
was working fine there, but it broke Active Record integrations since it
was just doing that for the `*_deliver` callbacks, and because of the
way the check was introduced, it was trying to use those same callbacks
inside AR, and they don't exist there, resulting in errors:

    NoMethodError (undefined method `before_deliver'...

This change to callbacks was introduced by Rails 5
(rails/rails#17227), which means we can safely
change the logic to `throw(:abort)` for both `deliver` and `create`
callbacks going forward.

Include some tests that simulate Active Record behavior through Active
Model and `create` callbacks, since we didn't have any specific test
coverage there.

Closes #56.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.