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

Needs extra caution when using within models #13

Closed
xTRiM opened this issue Feb 14, 2021 · 6 comments
Closed

Needs extra caution when using within models #13

xTRiM opened this issue Feb 14, 2021 · 6 comments

Comments

@xTRiM
Copy link

xTRiM commented Feb 14, 2021

We were looking at some strange bug where one test was weirdly affecting another - jobs enqueued in one test were seemingly trying to run in another.

As it turned out - there was a class method on the model, used for some bulk updates, which were enqueuing async jobs with after_commit {...} block.
Dev intended to run after_commit only inside of that bulk update method, but it actually was defined for the whole model, because the AR model has its own after_commit method.

Extracting that bulk method out from the AR model into a separate service solved the issue.

Probably makes sense to put some warning in the Readme about usage within AR models?

@Envek
Copy link
Owner

Envek commented Feb 14, 2021

Using this gem inside models is weird, because models have their own after_commit and this gem tries to reproduce AR's buitin method to allow its usage outside of models.

I thought that it is obvious: use built-in after_commit callback in models, bring after_commit_everywhere if you need to use it outside models.

However, as this gem uses builtin ActiveRecord's mechanisms (building fake records and registering in AR's after_commit callbacks queue), there shouldn't be any difference between them. Different behavior is a bug, I believe.

Please tell me more about your setup:

  • Ruby and Rails versions,
  • what database you're using,
  • whether you're using multiple databases,
  • any other fancy AR-related gems that can interfere
  • what you're using for executing async jobs?
  • what is your setup for testing async jobs (something like Sidekiq test mode?)

If you're able to provide executable Gist (like one for reproduction of bugs in ActiveRecord itself) it would be very helpful and appreciated.

@xTRiM
Copy link
Author

xTRiM commented Feb 14, 2021

You are absolutely right about after_commit in models.

It happened because we were so used to placing after_commit's in services (thank you for the gem!).
And it blew up after copying some service code to add "simple bulk updater" to the model (to cut the corner, which shouldn't happen but it did ;).

Actually, the gem itself doesn't even participate in this "bug."
It's a psychological bug, when the dev forgets about "outside of models" in the gem description and gets used to placing after_commit's everywhere ;)

Here's a simple gist which shows how it happened (and how it doesn't have to do anything with after_commit_everywhere itself).
https://gist.github.com/xTRiM/c10a492fc64da29109bad61716df83a4
Run with --seed 9083 for intended order.

@Envek
Copy link
Owner

Envek commented Feb 17, 2021

I played with your gist replacing puts with raise and swapping between AR's class-level callback and after_commit_everywhere and damn, yes, AR's class-level callback makes another test case to fail. This is so weird!

I guess that this gem's after_commit method behaves differently is that internally it uses instance-level callbacks, not class-level (basically, it uses ActiveRecord::Base#after_commit, not ActiveRecord::Base.after_commit).

This is apparently a bug in ActiveRecord itself, please create a new issue in the Ruby on Rails repository here: https://github.com/rails/rails/issues

Thank you for reporting this, it is interesting.

@Envek Envek closed this as completed Feb 17, 2021
@Envek
Copy link
Owner

Envek commented Feb 17, 2021

Oh, no, I was wrong, forget the nonsense that I wrote.

Actually there is a problem in following code:

class Post < ActiveRecord::Base
  def self.bulk_ops
    find_each do
      after_commit { raise "Some doesn't expect that to happen in future tests, but they should" }
    end
  end
end

This is the same as:

class Post < ActiveRecord::Base
  after_commit do
    raise "Some doesn't expect that to happen in SecondTest, but they should"
  end
end

So by calling class-level after_commit method you're effectively adding callback to future records.

That's why following test cases are failing. As they actually should.

@Envek
Copy link
Owner

Envek commented Feb 17, 2021

Added a note to the README

@xTRiM
Copy link
Author

xTRiM commented Feb 17, 2021

@Envek Incredible, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants