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

Question regarding after_commit behavior upon exception #34

Closed
kevink1103 opened this issue Dec 5, 2024 · 4 comments · Fixed by #35
Closed

Question regarding after_commit behavior upon exception #34

kevink1103 opened this issue Dec 5, 2024 · 4 comments · Fixed by #35
Labels
help wanted Extra attention is needed

Comments

@kevink1103
Copy link
Contributor

kevink1103 commented Dec 5, 2024

ActiveRecord::Base.transaction do
  AfterCommitEverywhere::after_commit { raise "should this raise and stop other callbacks?" }
  AfterCommitEverywhere::after_commit { ap "1" }
  AfterCommitEverywhere::after_commit { ap "2" }
  AfterCommitEverywhere::after_commit { ap "3" }
end

if executed, you'd get the following

"1"
"2"
"3"

RuntimeError: should this raise and stop other callbacks?

Is this behavior intended (every callback runs independently and doesn't affect the others)?
or should other following callbacks be cancelled and not executed when there is an exception just like rails's after_commit where it says:

class User < ActiveRecord::Base
  after_commit { raise "Intentional Error" }
  after_commit {
    # This won't get called because the previous after_commit raises an exception
    Rails.logger.info("This will not be logged")
  }
end

# ! If your callback code raises an exception, you'll need to rescue it and handle it within the callback in order to allow other callbacks to run.

and only result in

RuntimeError: should this raise and stop other callbacks?

without numbers being printed?


I noticed that if an exception in one callback needs to be propagated to other callbacks, the following lines of code

def committed!(*)
@handlers[:after_commit]&.call
end

should respect the should_run_callbacks keyword argument from Active Record.

https://github.com/rails/rails/blob/44673c80bde08f17659a709279972827ffa0ae64/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb#L315-L317

cc. @njw1204

@Envek
Copy link
Owner

Envek commented Dec 5, 2024

Thanks for raising this question (pun intended)

In ActiveRecord model, all defined callbacks are belong to one instance, so can be grouped together. In this gem for every after_commit invocation a new pseudo-record is instantiated, so they are independent from each other.

This behavior isn't documented or specifically decided to be that one, it just happened to be the current one.

We can try to “group” callbacks e.g. by current transaction (so all callbacks from all files will be grouped into a single “record”) or maybe by self (so e.g. every controller will have their own grouped callbacks, but service objects called from them will be independent).

I'm not sure which behavior is more desired.

@kevink1103
Copy link
Contributor Author

kevink1103 commented Dec 6, 2024

@Envek Thank you for prompt response!

In my opinion, it would be better to group callbacks by the current transaction (if there is one like the very first code snippet above).
To elaborate, if one of the after_commit callbacks in a transaction raises an exception, the succeeding callbacks should not execute.
This can give an option to users on whether one's callback error should be propagated to the others or not (by handling error within the callback itself).

-   def committed!(*)
-     @handlers[:after_commit]&.call
+   def committed!(should_run_callbacks: true)
+     if should_run_callbacks
+       @handlers[:after_commit]&.call
+     end
    end

What I suggest is to not running succeeding callbacks when active record tells you not to.

@Envek
Copy link
Owner

Envek commented Dec 6, 2024

Makes sense! Pull request are welcome! 😃

@Envek Envek added the help wanted Extra attention is needed label Dec 6, 2024
@kevink1103
Copy link
Contributor Author

kevink1103 commented Dec 9, 2024

Another issue with current implementation is that

ActiveRecord::Base.transaction do
  AfterCommitEverywhere::after_commit { raise "should this raise and stop other callbacks? - 1" }
  AfterCommitEverywhere::after_commit { ap "1" }
  AfterCommitEverywhere::after_commit { raise "should this raise and stop other callbacks? - 2" }
  AfterCommitEverywhere::after_commit { ap "2" }
  AfterCommitEverywhere::after_commit { ap "3" }
end

this would lead to

"1"

RuntimeError:
   should this raise and stop other callbacks? - 2
   # --- Caused by: ---
   # RuntimeError:
   #   should this raise and stop other callbacks? - 1

The entire after_commit execution within a transaction stops whenever there are two exceptions. (due to second exception raised inside ensure block here)
I think this behavior is unpredictable for most cases.

Envek pushed a commit that referenced this issue Dec 9, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
See discussion at #34 for details
@Envek Envek closed this as completed in #35 Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants