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

Stop swallowing errors on transaction after callbacks. #14488

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@arthurnn
Member

arthurnn commented Mar 26, 2014

Problem

When raising a standard error on after_commit / after_rollback, they are logged but swallowed afterwards. IMO, this is dangerous behaviour as people can just ignore the logs and never see those errors.

Solution

After re-work of the transaction model on rails 4+ this change is a trivial change. We just need to make sure we dont rollback the transaction after its is completed.

Drawbacks

When people upgrade rails to use the version with this commit(probably 4.2), they will probably start seeing lots of errors on tests, as before they were probably being swallowed, TBH I see this more as a advantage than a drawback.
Apps that use after_commit and need to ensure its behaviour even if the others records after_commits fail, could find trouble with this new behaviour. If thats a concern, we could add a new after_commit DSL, after_commit(:ensure) maybe.

[fixes #13460]

Would like to get most feedback as possible on this, as this changes rails behaviour.
review @rafaelfranca @tenderlove @dhh @chancancode @senny
cc @vipulnsward @fw42

Show outdated Hide outdated activerecord/test/cases/transaction_callbacks_test.rb
@@ -280,6 +280,26 @@ def @first.last_after_transaction_error; @last_transaction_error; end
assert_equal [:after_rollback], second.history
end
def test_after_commit_callback_should_not_swallow_errors
@first.after_commit_block{ fail "boom" }
assert_raise(RuntimeError) do

This comment has been minimized.

@tenderlove

tenderlove Mar 26, 2014

Member

This should be assert_raises

@tenderlove

tenderlove Mar 26, 2014

Member

This should be assert_raises

Show outdated Hide outdated activerecord/test/cases/transaction_callbacks_test.rb
def test_after_rollback_callback_should_not_swallow_errors
@first.after_rollback_block{ fail "boom" }
assert_raise(RuntimeError) do

This comment has been minimized.

@tenderlove

tenderlove Mar 26, 2014

Member

same.

@tenderlove

This comment has been minimized.

@tenderlove

tenderlove Mar 26, 2014

Member

Why is this a RuntimeError and not an ActiveRecord::Rollback error? I see an ActiveRecord::Rollback exception raised below, so I don't understand why this exception is different.

@tenderlove

tenderlove Mar 26, 2014

Member

Why is this a RuntimeError and not an ActiveRecord::Rollback error? I see an ActiveRecord::Rollback exception raised below, so I don't understand why this exception is different.

Show outdated Hide outdated activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -67,6 +71,9 @@ def joinable?
# This is a noop when there are no open transactions
def add_record(record)
end
def rollback
end

This comment has been minimized.

@tenderlove

tenderlove Mar 26, 2014

Member

What is this method for?

@tenderlove

tenderlove Mar 26, 2014

Member

What is this method for?

This comment has been minimized.

@arthurnn

arthurnn Mar 26, 2014

Member

you`re right, dont need this. (removing it...)

@arthurnn

arthurnn Mar 26, 2014

Member

you`re right, dont need this. (removing it...)

Show outdated Hide outdated activerecord/test/cases/transaction_callbacks_test.rb
def test_after_rollback_callback_should_not_swallow_errors
@first.after_rollback_block{ fail "boom" }
assert_raises(RuntimeError) do

This comment has been minimized.

@arthurnn

arthurnn Mar 26, 2014

Member

the reason this raises RuntimeError is because ActiveRecord::Rollback are swallowed in here https://github.com/arthurnn/rails/blob/stop_swallowing_errors/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L203 , and the actual error that i am catching is the one that fails on the after_rollback block.

@arthurnn

arthurnn Mar 26, 2014

Member

the reason this raises RuntimeError is because ActiveRecord::Rollback are swallowed in here https://github.com/arthurnn/rails/blob/stop_swallowing_errors/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb#L203 , and the actual error that i am catching is the one that fails on the after_rollback block.

This comment has been minimized.

@vipulnsward

vipulnsward Mar 27, 2014

Member

👍 exactly what the PR should achieve.

@vipulnsward

vipulnsward Mar 27, 2014

Member

👍 exactly what the PR should achieve.

This comment has been minimized.

@tenderlove

tenderlove Apr 1, 2014

Member

@arthurnn so you're trying to catch an exception from the after_rollback_block? Catching the exception that fail raises is really confusing. Can you raise a specific exception, and catch that exception so we can understand what is going on?

@tenderlove

tenderlove Apr 1, 2014

Member

@arthurnn so you're trying to catch an exception from the after_rollback_block? Catching the exception that fail raises is really confusing. Can you raise a specific exception, and catch that exception so we can understand what is going on?

This comment has been minimized.

@arthurnn

arthurnn Apr 1, 2014

Member

For sure! will do that.

@arthurnn

arthurnn Apr 1, 2014

Member

For sure! will do that.

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Mar 26, 2014

Member

just edited the description to add another drawback that could be a concern.

Member

arthurnn commented Mar 26, 2014

just edited the description to add another drawback that could be a concern.

Show outdated Hide outdated activerecord/test/cases/transaction_callbacks_test.rb
@first.after_commit_block{|r| r.last_after_transaction_error = :commit; raise "fail!";}
@first.after_rollback_block{|r| r.last_after_transaction_error = :rollback; raise "fail!";}
@first.after_commit_block{|r| r.last_after_transaction_error = :commit;}
@first.after_rollback_block{|r| r.last_after_transaction_error = :rollback;}

This comment has been minimized.

@fw42

fw42 Mar 26, 2014

Contributor

Nitpick: Don't need those semicolons anymore :)

@fw42

fw42 Mar 26, 2014

Contributor

Nitpick: Don't need those semicolons anymore :)

This comment has been minimized.

@arthurnn

arthurnn Mar 26, 2014

Member

good catch

@arthurnn

arthurnn Mar 26, 2014

Member

good catch

@@ -214,7 +214,7 @@ def within_new_transaction(options = {}) #:nodoc:
begin
commit_transaction unless error
rescue Exception
rollback_transaction
rollback_transaction unless @transaction.state.completed?

This comment has been minimized.

@fw42

fw42 Mar 26, 2014

Contributor

Why is this needed?

@fw42

fw42 Mar 26, 2014

Contributor

Why is this needed?

This comment has been minimized.

@arthurnn

arthurnn Mar 26, 2014

Member

because we need to make sure we only rollback the transaction if the transaction is not committed, or rollbacked already, otherwise the db will 💥

@arthurnn

arthurnn Mar 26, 2014

Member

because we need to make sure we only rollback the transaction if the transaction is not committed, or rollbacked already, otherwise the db will 💥

This comment has been minimized.

@fw42

fw42 Mar 26, 2014

Contributor

Right, and that wasn't necessary before because it didn't raise and thus never took this code path. Got it. 👍

@fw42

fw42 Mar 26, 2014

Contributor

Right, and that wasn't necessary before because it didn't raise and thus never took this code path. Got it. 👍

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 Mar 26, 2014

Contributor

👍 would love to see this merged

Contributor

fw42 commented Mar 26, 2014

👍 would love to see this merged

Show outdated Hide outdated activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
end
end
def commit_records
@state.set_state(:committed)
records.uniq.each do |record|
begin

This comment has been minimized.

@vipulnsward

vipulnsward Mar 27, 2014

Member

@carlosantoniodasilva had mentioned we should log these errors too. I hope raising them, still logs them.

@vipulnsward

vipulnsward Mar 27, 2014

Member

@carlosantoniodasilva had mentioned we should log these errors too. I hope raising them, still logs them.

This comment has been minimized.

@rafaelfranca

rafaelfranca Apr 2, 2014

Member

I don't see why we need to log and raise. I think Rails will already take care of logging the exception. @carlosantoniodasilva what are your concerns?

@rafaelfranca

rafaelfranca Apr 2, 2014

Member

I don't see why we need to log and raise. I think Rails will already take care of logging the exception. @carlosantoniodasilva what are your concerns?

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva May 7, 2014

Member

I really don't remember 😄.

@vipulnsward do you remember where we discussed this?

@carlosantoniodasilva

carlosantoniodasilva May 7, 2014

Member

I really don't remember 😄.

@vipulnsward do you remember where we discussed this?

@robin850 robin850 added this to the 4.2.0 milestone Mar 30, 2014

Show outdated Hide outdated activerecord/test/cases/transaction_callbacks_test.rb
end
def test_after_rollback_callback_should_not_swallow_errors
error_class = Class.new(StandardError)

This comment has been minimized.

@arthurnn

arthurnn Apr 1, 2014

Member

@tenderlove done. added a custom error class.

@arthurnn

arthurnn Apr 1, 2014

Member

@tenderlove done. added a custom error class.

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Apr 7, 2014

Member

Also pulling @jeremy in this, as he commented on the ticket about it, 3 years ago.

Member

arthurnn commented Apr 7, 2014

Also pulling @jeremy in this, as he commented on the ticket about it, 3 years ago.

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Apr 16, 2014

Member

ping , any final concerns about this?

Member

arthurnn commented Apr 16, 2014

ping , any final concerns about this?

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Apr 16, 2014

Member

I haven't had a chance to look into this yet (not sure if I'd be able to before Rails Conf =/), but I just wanted to point you to this thread in case it's relevant for your change. Some of the rollback behaviour is in a pretty bizarre state due to the most-likely-wrong documentation.

Need to do a deeper investigation across the codebase to see what the code actually does and how we should fix this to get back to a consistent state. Might need some help from you on that later :P

Member

chancancode commented Apr 16, 2014

I haven't had a chance to look into this yet (not sure if I'd be able to before Rails Conf =/), but I just wanted to point you to this thread in case it's relevant for your change. Some of the rollback behaviour is in a pretty bizarre state due to the most-likely-wrong documentation.

Need to do a deeper investigation across the codebase to see what the code actually does and how we should fix this to get back to a consistent state. Might need some help from you on that later :P

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode May 7, 2014

Member

Confirmed this is independent from the nested transaction bug.

Member

chancancode commented May 7, 2014

Confirmed this is independent from the nested transaction bug.

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn
Member

arthurnn commented May 13, 2014

@chancancode :shipit: this?

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode May 13, 2014

Member

I just caught up and now I understood what this is about 😄

I'm slightly concerned that this is going to be a potentially significant change in applications where this happens. Nested transactions would work differently for example (because it'll now re-raise the error and cause the parent transaction to rollback as well). Another example might be that people were doing something that could fail in the callbacks (e.g. remote logging? idk) and they were relying on the fact that the errors are swallowed.

The conservative side of me feels like that it's safer to go through a deprecation cycle for something like this. Alternatively, we can introduce a flag for this, e.g. config.active_record.errors_in_transactional_callbacks = :log / :raise. Then we can emit a deprecation warning when this is not set, and warn that the default will be changed to :raise in the future and the :log option will be removed eventually.

I'm all 👍 for the new behaviour, just unsure on how to get there :)

Member

chancancode commented May 13, 2014

I just caught up and now I understood what this is about 😄

I'm slightly concerned that this is going to be a potentially significant change in applications where this happens. Nested transactions would work differently for example (because it'll now re-raise the error and cause the parent transaction to rollback as well). Another example might be that people were doing something that could fail in the callbacks (e.g. remote logging? idk) and they were relying on the fact that the errors are swallowed.

The conservative side of me feels like that it's safer to go through a deprecation cycle for something like this. Alternatively, we can introduce a flag for this, e.g. config.active_record.errors_in_transactional_callbacks = :log / :raise. Then we can emit a deprecation warning when this is not set, and warn that the default will be changed to :raise in the future and the :log option will be removed eventually.

I'm all 👍 for the new behaviour, just unsure on how to get there :)

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn May 13, 2014

Member

@chancancode I share the same concerns, (that what I tried to explained on the 'Drawbacks' section). I really like the idea of deprecate this on 4.2 , with a config, and remove it in future versions... I can do it ❤️ 😸

Member

arthurnn commented May 13, 2014

@chancancode I share the same concerns, (that what I tried to explained on the 'Drawbacks' section). I really like the idea of deprecate this on 4.2 , with a config, and remove it in future versions... I can do it ❤️ 😸

Show outdated Hide outdated activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
rescue => e
record.logger.error(e) if record.respond_to?(:logger) && record.logger
end
record.rolledback!(parent.closed?)
end

This comment has been minimized.

@chancancode

chancancode May 13, 2014

Member

Another subtle behaviour change: before this change the loop will always finish execution, after this change if one of the item errored for whatever reason then the loop will not complete. This matters if you caught the exception manually – in that case some of the records will be in an inconsistent state

@chancancode

chancancode May 13, 2014

Member

Another subtle behaviour change: before this change the loop will always finish execution, after this change if one of the item errored for whatever reason then the loop will not complete. This matters if you caught the exception manually – in that case some of the records will be in an inconsistent state

This comment has been minimized.

@chancancode

chancancode May 13, 2014

Member

(I just noticed that the committed! side has the same semantics, but still, I suppose there are generally more states clean up to do after a rollback compared to a commit)

@chancancode

chancancode May 13, 2014

Member

(I just noticed that the committed! side has the same semantics, but still, I suppose there are generally more states clean up to do after a rollback compared to a commit)

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn May 15, 2014

Member

@chancancode I added more tests, do you mind double checking if they make sense, and if they cover what we discussed?
(test_after_rollback_callback_when_raise_should_restore_state is not working yet, but if it make sense, I will make it 💚 )

Member

arthurnn commented May 15, 2014

@chancancode I added more tests, do you mind double checking if they make sense, and if they cover what we discussed?
(test_after_rollback_callback_when_raise_should_restore_state is not working yet, but if it make sense, I will make it 💚 )

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn May 15, 2014

Member

@chancancode I added the option in AR, and also added enough tests to cover what we discussed. Please let me know if I missed any case.

Member

arthurnn commented May 15, 2014

@chancancode I added the option in AR, and also added enough tests to cover what we discussed. Please let me know if I missed any case.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode May 15, 2014

Member

Related: @arthurnn what is the relation between this and #11118? Sounds like it's already fixed long before this and can be closed

Member

chancancode commented May 15, 2014

Related: @arthurnn what is the relation between this and #11118? Sounds like it's already fixed long before this and can be closed

records.uniq.each do |record|
begin
record.rolledback!(parent.closed?)
record.rolledback! unless error

This comment has been minimized.

@chancancode

chancancode May 15, 2014

Member

Can you explain this a little bit?

@chancancode

chancancode May 15, 2014

Member

Can you explain this a little bit?

This comment has been minimized.

@chancancode

chancancode May 15, 2014

Member

hmm, nvm, I think I got it

@chancancode

chancancode May 15, 2014

Member

hmm, nvm, I think I got it

if :raise == ActiveRecord::Base.errors_in_transactional_callbacks
error = e
else
record.logger.error(e) if record.respond_to?(:logger) && record.logger

This comment has been minimized.

@chancancode

chancancode May 16, 2014

Member

I think we should always log. For several reasons:

  1. People might have already have code set up to parse the logs for these errors and alert the developers, so always logging the errors will ensure those code continue to work. I don't feel strongly about this point though.
  2. There could be multiple records/callbacks here, and we can only raise one error. If more than one of them raised, the rest will be masked.
  3. Most importantly: if the rollback was caused by a different exception to begin with, then we should always raise the original error – in that case the errors in the callbacks will again be masked. This is probably my biggest concern.

Perhaps this is what @carlosantoniodasilva was thinking? cc @vipulnsward

@chancancode

chancancode May 16, 2014

Member

I think we should always log. For several reasons:

  1. People might have already have code set up to parse the logs for these errors and alert the developers, so always logging the errors will ensure those code continue to work. I don't feel strongly about this point though.
  2. There could be multiple records/callbacks here, and we can only raise one error. If more than one of them raised, the rest will be masked.
  3. Most importantly: if the rollback was caused by a different exception to begin with, then we should always raise the original error – in that case the errors in the callbacks will again be masked. This is probably my biggest concern.

Perhaps this is what @carlosantoniodasilva was thinking? cc @vipulnsward

This comment has been minimized.

@chancancode

chancancode May 16, 2014

Member

Alternatively, we can try to figure out when these cases occurs, and only log/warn when an error is actually being masked. I suspect that might be quite difficult though.

@chancancode

chancancode May 16, 2014

Member

Alternatively, we can try to figure out when these cases occurs, and only log/warn when an error is actually being masked. I suspect that might be quite difficult though.

This comment has been minimized.

@arthurnn

arthurnn May 16, 2014

Member

My answer to your points:

  1. Yes, we probably should log it all the time. I think that wont hurt. (maybe we remove that in the future., but lets go baby-steps for now)
  2. There is no way to more then one error be raised. because we only run the callbacks if the error object is nil, so after the first error is set, the callbacks wont run.
  3. If there is a different exception, we should raise the last one, which is more important IMO, as, thats what the user would expect to fix first.
@arthurnn

arthurnn May 16, 2014

Member

My answer to your points:

  1. Yes, we probably should log it all the time. I think that wont hurt. (maybe we remove that in the future., but lets go baby-steps for now)
  2. There is no way to more then one error be raised. because we only run the callbacks if the error object is nil, so after the first error is set, the callbacks wont run.
  3. If there is a different exception, we should raise the last one, which is more important IMO, as, thats what the user would expect to fix first.

This comment has been minimized.

@jonleighton

jonleighton Jul 2, 2014

Member

I think we should solve (3) with documentation: if you use after_rollback you should be very careful that your code won't raise another exception.

@jonleighton

jonleighton Jul 2, 2014

Member

I think we should solve (3) with documentation: if you use after_rollback you should be very careful that your code won't raise another exception.

This comment has been minimized.

@arthurnn

arthurnn Aug 18, 2014

Member

👍 agree with @jonleighton . If you have another error happening on after_rollback you will want to fix that one too, so that last error is important.

@arthurnn

arthurnn Aug 18, 2014

Member

👍 agree with @jonleighton . If you have another error happening on after_rollback you will want to fix that one too, so that last error is important.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode May 16, 2014

Member

Actually – there is a bigger problem with this. We cannot raise errors within Transaction#commit and Transaction#rollback. The return value of those methods are used inside commit_transaction and rollback_transaction to set to keep adapter's current transaction in-sync inside @transaction. If Transaction#commit or Transaction#rollback raises, they cannot return a value to the adapter and that value will be out-of-sync.

It seems like it would require a pretty big refactor if we would like to support this new behaviour...

Member

chancancode commented May 16, 2014

Actually – there is a bigger problem with this. We cannot raise errors within Transaction#commit and Transaction#rollback. The return value of those methods are used inside commit_transaction and rollback_transaction to set to keep adapter's current transaction in-sync inside @transaction. If Transaction#commit or Transaction#rollback raises, they cannot return a value to the adapter and that value will be out-of-sync.

It seems like it would require a pretty big refactor if we would like to support this new behaviour...

arthurnn added some commits Mar 26, 2014

Add active_record.errors_in_transactional_callbacks config
Add active_record.errors_in_transactional_callbacks option to active
record. Also add a warning message for when the option is not set.

Here is what the values 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.
Make sure rollbacks state gets restored
When  config is set to raise, we need still to make sure the records are properly restored
@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Jul 2, 2014

Member

@arthurnn I'd like to get this merged. Will add a few comments which I have.

Member

jonleighton commented Jul 2, 2014

@arthurnn I'd like to get this merged. Will add a few comments which I have.

`:raise` : If any error should occur in after_commit/after_rollback callbacks, it will not be swallowed.
EOF
end

This comment has been minimized.

@jonleighton

jonleighton Jul 2, 2014

Member

I think this places too much burden on people who may never use after_commit and after_rollback callbacks. I think we should instead show this deprecation when after_commit/after_rollback callbacks are declared.

Also, I think we should move towards not having this option at all: we should eventually always just raise. If people want to suppress certain exceptions then they can do that in their callbacks without complicating AR's own functionality.

Therefore I think the config option should indicate this: in the next version the the default will be :raise and setting :log will have no effect. And setting :log in this version should continue to show a deprecation warning along the lines of: "the :log behaviour is deprecated and you should migrate to :raise".

@jonleighton

jonleighton Jul 2, 2014

Member

I think this places too much burden on people who may never use after_commit and after_rollback callbacks. I think we should instead show this deprecation when after_commit/after_rollback callbacks are declared.

Also, I think we should move towards not having this option at all: we should eventually always just raise. If people want to suppress certain exceptions then they can do that in their callbacks without complicating AR's own functionality.

Therefore I think the config option should indicate this: in the next version the the default will be :raise and setting :log will have no effect. And setting :log in this version should continue to show a deprecation warning along the lines of: "the :log behaviour is deprecated and you should migrate to :raise".

This comment has been minimized.

@arthurnn

arthurnn Jul 18, 2014

Member

I am 👍 with that.. @chancancode thoughts?

@arthurnn

arthurnn Jul 18, 2014

Member

I am 👍 with that.. @chancancode thoughts?

This comment has been minimized.

@chancancode

chancancode Jul 18, 2014

Member

you should listen to @jonleighton ;) 👍

@chancancode

chancancode Jul 18, 2014

Member

you should listen to @jonleighton ;) 👍

record.logger.error(e) if record.respond_to?(:logger) && record.logger
end
ensure
record.force_clear_transaction_record_state

This comment has been minimized.

@jonleighton

jonleighton Jul 2, 2014

Member

What's the reason to move this out of the committed! method?

@jonleighton

jonleighton Jul 2, 2014

Member

What's the reason to move this out of the committed! method?

This comment has been minimized.

@arthurnn

arthurnn Aug 18, 2014

Member

fixed that, didnt move that anymore on my new PR

@arthurnn

arthurnn Aug 18, 2014

Member

fixed that, didnt move that anymore on my new PR

end
end
raise error if error

This comment has been minimized.

@jonleighton

jonleighton Jul 2, 2014

Member

I don't think we should suppress the error until we've iterated each record. I think it's perfectly acceptable to just raise the first error that occurs, potentially stopping future callbacks. Doing so makes the flow of things clearer from the end-users perspective, and it would make this code simpler as well.

@jonleighton

jonleighton Jul 2, 2014

Member

I don't think we should suppress the error until we've iterated each record. I think it's perfectly acceptable to just raise the first error that occurs, potentially stopping future callbacks. Doing so makes the flow of things clearer from the end-users perspective, and it would make this code simpler as well.

This comment has been minimized.

@arthurnn

arthurnn Jul 18, 2014

Member

I totally agree with that. ( actually that was my first implementation ), However I changed to raise after a few iterations and reviews on this PR (dont clearly remember where/why).
Another reason would be that we are going to be changing behaviour, as if you have 3 after_commits to run, and the first one raises, the other 2 wont run, which is not what happens ATM.

@arthurnn

arthurnn Jul 18, 2014

Member

I totally agree with that. ( actually that was my first implementation ), However I changed to raise after a few iterations and reviews on this PR (dont clearly remember where/why).
Another reason would be that we are going to be changing behaviour, as if you have 3 after_commits to run, and the first one raises, the other 2 wont run, which is not what happens ATM.

This comment has been minimized.

@jonleighton

jonleighton Jul 18, 2014

Member

Another reason would be that we are going to be changing behaviour, as if you have 3 after_commits to run, and the first one raises, the other 2 wont run, which is not what happens ATM.

I think it's fine to change this, as the not-raising behaviour is already changing :) [and we have a config flag for the upgrade]

@jonleighton

jonleighton Jul 18, 2014

Member

Another reason would be that we are going to be changing behaviour, as if you have 3 after_commits to run, and the first one raises, the other 2 wont run, which is not what happens ATM.

I think it's fine to change this, as the not-raising behaviour is already changing :) [and we have a config flag for the upgrade]

This comment has been minimized.

@arthurnn

arthurnn Aug 18, 2014

Member

👍 for changing the behaviour.. Will change this.

@arthurnn

arthurnn Aug 18, 2014

Member

👍 for changing the behaviour.. Will change this.

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Jul 25, 2014

Member

I will rebase this, and fix it after we merge the transaction stack PR #16284

Member

arthurnn commented Jul 25, 2014

I will rebase this, and fix it after we merge the transaction stack PR #16284

@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn Aug 18, 2014

Member

Closing this, as the new PR #16537 should have this commits + the feedback gather on theses discussions.

Member

arthurnn commented Aug 18, 2014

Closing this, as the new PR #16537 should have this commits + the feedback gather on theses discussions.

@arthurnn arthurnn closed this Aug 18, 2014

@arthurnn arthurnn deleted the arthurnn:stop_swallowing_errors branch Aug 18, 2014

faun added a commit to zinedistro/zinedistro that referenced this pull request Jan 29, 2015

Improve error handling in transaction callbacks
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

Improve error handling in transaction callbacks
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

Improve error handling in transaction callbacks
See [#14488] and [#16537] for more details.

[#14488]: [rails/rails#14488]
[#16537]: [rails/rails#16537]

d1rtyvans added a commit to alookatommorow/west-coast-skateparks that referenced this pull request Mar 9, 2016

@shade9795

This comment has been minimized.

Show comment
Hide comment
@shade9795

shade9795 Jan 12, 2018

how can I access the address
activerecord/test/cases/transaction_callbacks_test.rb

shade9795 commented Jan 12, 2018

how can I access the address
activerecord/test/cases/transaction_callbacks_test.rb

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