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

Add option to stop swallowing errors on callbacks. #16537

Merged
merged 1 commit into from Aug 18, 2014

Conversation

@arthurnn
Copy link
Member

arthurnn commented Aug 18, 2014

Seconds take of #14488 .

After the transaction refactor, we can cleanly apply this patch, as we have a better control of the stack of transactions, and we know that even when raising errors, the transaction wont leaky into the stack.

Description

Add option to stop swallowing standard errors on after_commit and after_rollback.
Currently the option will be +false+ and throw a warning untill set it to +true+.

Example:

# For not swallow errors in after_commit/after_rollback callbacks.
config.active_record.errors_in_transactional_callbacks = true

The option should be removed in future versions of Rails, as the desired
behaviour is not to swallow exceptions.

[fixes #13460]

review @jonleighton @rafaelfranca @tenderlove @chancancode @senny

@arthurnn
arthurnn reviewed Aug 18, 2014
View changes
activerecord/lib/active_record/transactions.rb Outdated
@@ -375,6 +363,21 @@ def restore_transaction_record_state(force = false) #:nodoc:
end
end

protected

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 18, 2014

Author Member

this code didnt change, just had to move it, so I can have access to clear_transaction_record_state force_clear_transaction_record_state restore_transaction_record_state

This comment has been minimized.

Copy link
@chancancode

chancancode Aug 18, 2014

Member

See above

connection.commit_db_transaction
super

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 18, 2014

Author Member

only should call super(which changes the state of the transaction), after the actual DB call had happened. (same applies to the rollback method)

@arthurnn
arthurnn reviewed Aug 18, 2014
View changes
activerecord/lib/active_record/connection_adapters/abstract/transaction.rb Outdated
Here's what they mean:
`:log` : after_commit/after_rollback callbacks would only log if there is an error raised in the callback.
`:raise` : If any error should occur in after_commit/after_rollback callbacks, it will not be swallowed.
EOF

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 18, 2014

Author Member

I am not strong about the wording in here... Fell free to change it, as my english might not be good enough.

This comment has been minimized.

Copy link
@chancancode

chancancode Aug 18, 2014

Member

Nit: let's make this a boolean ActiveRecord::Base.raise_in_transactional_callbacks and default it to false. IMO, it doesn't make sense to provide the :log option to silence the warning if we are just going to remove it in the next release.

It could say something along the lines of "Currently, this is how it works. In the future, this is how it would work. Set blah to true to opt into the new behavior and remove this warning."

@chancancode
chancancode reviewed Aug 18, 2014
View changes
activerecord/lib/active_record/connection_adapters/abstract/transaction.rb Outdated
begin
record.committed!
true

This comment has been minimized.

Copy link
@chancancode

chancancode Aug 18, 2014

Member

what is this for? (and why is it not needed for the rollback case?)

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 18, 2014

Author Member

sorry.. this was a left over.. not necessary!

@chancancode
chancancode reviewed Aug 18, 2014
View changes
activerecord/lib/active_record/connection_adapters/abstract/transaction.rb Outdated
rescue => e
raise if :raise == ActiveRecord::Base.errors_in_transactional_callbacks
deprecation_warning_on_log

This comment has been minimized.

Copy link
@chancancode

chancancode Aug 18, 2014

Member

Maybe we should emit this warning at the point where people define a transactional callback, to make it more likely that someone who cares about this will actually see it.

This comment has been minimized.

Copy link
@chancancode

chancancode Aug 18, 2014

Member

(transactional callbacks = after_commit & after_rollback, right? Or is after_create actually affected somehow?)

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 18, 2014

Author Member

only after_commit & after_rollback

@chancancode
chancancode reviewed Aug 18, 2014
View changes
activerecord/lib/active_record/connection_adapters/abstract/transaction.rb Outdated
ensure
ite.each do |i|
i.restore_transaction_record_state full_rollback?
i.clear_transaction_record_state

This comment has been minimized.

Copy link
@chancancode

chancancode Aug 18, 2014

Member

wdyt if we make this i.rolledback! full_rollback?, false ? (def rolledback(force_restore_state = false, run_callbacks = true), we can turn that into kwargs in the future)

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 18, 2014

Author Member

I like it!

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 18, 2014

Author Member

run_callbacks is the name of the method, so I cannot use that as the name of the parameter =( ... any other suggestion ?

@chancancode
chancancode reviewed Aug 18, 2014
View changes
activerecord/lib/active_record/connection_adapters/abstract/transaction.rb Outdated
rescue => ex
raise
ensure
ite.each(&:force_clear_transaction_record_state) if ex

This comment has been minimized.

Copy link
@chancancode

chancancode Aug 18, 2014

Member

Same as above, (committed(false)). I would like to keep these housekeeping logic in one place and not duplicate it

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 18, 2014

Author Member

Agree with out.. just didnt like to pass the argument too.. but I guess it is better.

@arthurnn
Copy link
Member Author

arthurnn commented Aug 18, 2014

@chancancode I guess I addressed all your concerns:

  • Remove unnecessary true
  • Emit warning when the definition of after_commit/after_rollback happens.
  • Add extra argument to rolledback! and committed to not run callbacks. (change code to use them, and dont change visibility of internal methods)
  • Change option to errors_in_transactional_callbacks and a bool.
  • Warning will always show until the option is set to true.
  • Re-wording of the warning message.
@chancancode
chancancode reviewed Aug 18, 2014
View changes
activerecord/lib/active_record/transactions.rb Outdated
@@ -3,11 +3,21 @@ module ActiveRecord
module Transactions
extend ActiveSupport::Concern
ACTIONS = [:create, :destroy, :update]
CALLBACK_WARN_MESSAGE = <<-EOF
Currently `config.active_record.raise_in_transactional_callbacks` is set to false,

This comment has been minimized.

Copy link
@chancancode

chancancode Aug 18, 2014

Member

I'll start with describing the behavior (i.e. errors are only logged) and the future change. The "option" is really just an opt-in flag and should be mentioned last. (Same for the changelog entry)

But we can also improve this after merging.

Currently, Active Record will rescue any errors raised within
after_rollback/after_create callbacks and print them to the
logs. Next versions of rails will not rescue those errors anymore,
and just bubble them up, as the other callbacks.

This adds a opt-in flag to enable that behaviour, of not rescuing
the errors.
    Example:
      # For not swallow errors in after_commit/after_rollback
      config.active_record.errors_in_transactional_callbacks = true

[fixes #13460]
@arthurnn
Copy link
Member Author

arthurnn commented Aug 18, 2014

@chancancode Warning message, CHANGELOG, and commit message updated.

chancancode added a commit that referenced this pull request Aug 18, 2014
Add option to stop swallowing errors on callbacks.
@chancancode chancancode merged commit 879dde9 into rails:master Aug 18, 2014
1 check was pending
1 check was pending
continuous-integration/travis-ci The Travis CI build is in progress
Details
@arthurnn arthurnn deleted the arthurnn:stop_swallowing_errors_2 branch Aug 18, 2014
@@ -63,13 +67,19 @@ def rollback
end

def rollback_records
records.uniq.each do |record|
ite = records.uniq

This comment has been minimized.

Copy link
@jonleighton

jonleighton Aug 19, 2014

Member

Is ite an acronym for something?

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 20, 2014

Author Member

iterator ...

ensure
ite.each do |i|
i.rolledback!(full_rollback?, false)
end

This comment has been minimized.

Copy link
@jonleighton

jonleighton Aug 19, 2014

Member

I found the logic here hard to follow. I think the issue is that the rollback!/committed! methods are doing two things which we wish to be able to separate:

  1. run callbacks
  2. change internal state of the object

Perhaps we could split out these things into separate methods? Then we could do the state-changing on each record before running any callbacks (perhaps). Or at least avoid the boolean (which makes it non-obvious what is going on).

This comment has been minimized.

Copy link
@chancancode

chancancode Aug 19, 2014

Member

The plan was to switch the boolean arg into kwarg, which could happen once we branched 4-2-stable. But it seems like splitting that up is a better plan 👍

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 20, 2014

Author Member

I will split that up!

def committed! #:nodoc:
run_callbacks :commit if destroyed? || persisted?
def committed!(should_run_callbacks = true) #:nodoc:
run_callbacks :commit if should_run_callbacks && destroyed? || persisted?

This comment has been minimized.

Copy link
@jonleighton

jonleighton Aug 19, 2014

Member

The precedence here is (a && b) || c, but I think you intended a && (b || c)?

This comment has been minimized.

Copy link
@arthurnn

arthurnn Aug 20, 2014

Author Member

you are right.. will fix it.

@jonleighton
Copy link
Member

jonleighton commented Aug 19, 2014

Perhaps new apps should be generated with the config option already set?

@chancancode
Copy link
Member

chancancode commented Aug 19, 2014

@jonleighton yep, it's been added in c7c2df2

(Thanks for taking your time to help review this by the way!)

pjambet added a commit to harrystech/rails-pipeline that referenced this pull request Oct 31, 2014
By default Rails swallows exception in after_{commit/rollback} hooks,
therefore prevent specs to fail.
By moving the callbacks inside the transaction in test environment any
exceptions thrown will correctly be bubbled up.

There is an option to fix that in rails 4.2
rails/rails#16537
faun added a commit to zinedistro/zinedistro that referenced this pull request Jan 29, 2015
See [#14488] [1] and [#16537] [2] for more details.

[1]: [rails/rails#14488]
[2]: [rails/rails#16537]
faun added a commit to zinedistro/zinedistro that referenced this pull request Jan 29, 2015
See [#14488] and [#16537] for more details.

[#14488]: [rails/rails#14488]
[#16537]: [rails/rails#16537]
faun added a commit to zinedistro/zinedistro that referenced this pull request Jan 29, 2015
See [#14488] and [#16537] for more details.

[#14488]: [rails/rails#14488]
[#16537]: [rails/rails#16537]
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.

3 participants
You can’t perform that action at this time.