Isolated problem to using Mobility gem and Good Job together

[leehericks]

Can you pull, update and run this and see that you also get the problem now?

[leehericks]

bundle install
rails g mobility:install
rails db:migrate

[leehericks]

@bensheldon if you can confirm the problem, we might want to invite https://github.com/shioyama to this project?

[leehericks]

The two gems worked together fine in Rails 6.1.4.1

[leehericks]

Of course the point is also this only occurs in Puma cluster mode (web concurrency)

[bensheldon]

👍 I'll take a look later tonight

[leehericks]

Thanks! I added an Issue on Mobility as well:
shioyama/mobility#549

[bensheldon]

Reproduced! Now to figure out what's happening.

[leehericks]

Alpha Rails, Puma in cluster mode, A tale of two gems....where to start? haha

[shioyama]

Very very strange... I'll have a look too.

[shioyama]

So it happens even without config/initializers/mobility.rb, which is very strange. So Mobility doesn't even need to initialize itself to somehow trigger the issue.

[shioyama]

I wonder if this one could be implicated: shioyama/mobility#520

It defines ruby2_keywords on the global scope.

[shioyama]

Ok, I think the problem is with Mobility's Rails generators. If I comment out this line, the request goes through:

https://github.com/shioyama/mobility/blob/99579bb5f3bf9b3588fe5e7319bcb1b9e83131ec/lib/mobility.rb#L93

  require "rails/generators/mobility/generators" if defined?(Rails)

This code is not carefully watched and hard to test, so it kind of makes sense that something strange would be missed there.

[shioyama]

Ok, specifically it's this file:

lib/rails/generators/mobility/backend_generators/base.rb
https://github.com/shioyama/mobility/blob/99579bb5f3bf9b3588fe5e7319bcb1b9e83131ec/lib/rails/generators/mobility/backend_generators/base.rb

I suspect there are some things that changed in Rails that makes these things fail in some kind of weird way.

I need to find a way to test this stuff better...

[shioyama]

Ok crazy thing, it's this line:

https://github.com/shioyama/mobility/blob/99579bb5f3bf9b3588fe5e7319bcb1b9e83131ec/lib/rails/generators/mobility/backend_generators/base.rb#L49

      delegate :connection, to: ::ActiveRecord::Base

If I replace that with:

      def connection
        ::ActionRecord::Base.connection
      end

then the problem goes away. Although the two are functionally the same, the difference being that in the current implementation AR::Base is immediately loaded.

And indeed it seems the problem here is simply referencing ActiveRecord::Base. I have no idea why that would be.

[shioyama]

I think this should fix it: shioyama/mobility#550

It doesn't change anything other than avoids referencing ActiveRecord::Base immediately when loading Mobility, which is a good thing so happy to ship it.

The weird thing though is that the file is only loaded if defined?(Rails) is true. I had assumed that if Rails was loaded then ActiveRecord::Base would also be loaded, so delegate should then be fine. But for some reason that seems not to be the case (anymore?). It appears to be already loaded before that line. So I don't know wtf is going on honestly.

[bensheldon]

@shioyama thank for digging into this. This matches up with deadlocks I've experienced in the past with GoodJob, and what I've done to fix them:

• Configure GoodJob via Rails.application.config instead of recommending GoodJob::Adapter.new good_job#199
• Defer accessing ActiveRecord primary_key in Lockable good_job#293

The intuition that I've been coding against is that static code inside of gems shouldn't reference ActiveRecord unless it's within a Rails-controlled context (e.g. a Railtie initializer, or Rails Executor/Reloader). This seems to be because, in the Development environment with lazy constant loading, ActiveRecord isn't necessarily fully initialized until Rails is fully initialized (e.g. after_initialize).

I've seen this problem manifest in GoodJob quite a bit, but I'm not sure if that's because GoodJob is aggressive with instantiating worker threads and database connections immediately after Rails initialization (and thus race conditions are most visible to GoodJob), or if GoodJob is doing something wrong with Rails Executors/ActiveRecord Connections, or if there is a bug in Rails. I do think it seems surprising and against common practice for gems to be so hands-off ActiveRecord constants.

That said, searching the Rails repo for "ActiveRecord deadlock" brings up quite a lot of issues with similar insights, for example: rails/rails#34310 (comment)

Lastly, I'm curious how you were able to isolate down the problem so quickly; I am impressed.

[leehericks]

@shioyama Yes I'm also really impressed!

But why does this only manifest when the two gems are both installed? 🤔

[shioyama]

Lastly, I'm curious how you were able to isolate down the problem so quickly; I am impressed.

Manual code bisecting: bundle open mobility, and comment out half the stuff, run rails s check if it hangs, if not comment out more, etc. Gotta be careful about modifying gem code though to clean up after yourself or you can end up with really weird bugs. I probably should have used path: in the Gemfile and pointed to my local copy of mobility.

But why does this only manifest when the two gems are both installed? thinking

As far as Mobility is concerned it's just the referencing of ActiveRecord::Base. You could create a blank gem which only has one line with ActiveRecord::Base and presumably it would do the same thing.

I don't know enough about changes in Rails 7 or GoodJob to say more about that side of the picture though.

[shioyama]

p.s. released 1.2.5 with the fix in shioyama/mobility#550.

[leehericks]

Thank you!