Resolve Rails Reloader and ActiveRecord middleware incompatibility by sj26 · Pull Request #3166 · sidekiq/sidekiq

sj26 · 2016-09-28T05:12:32Z

The new Rails reloader enables the query cache within every yield and expects to have access to the current connection that it enabled the query cache for at the end of the block to clear the cache and then release the connection. The Sidekiq ActiveRecord middleware which is added automatically releases the connection earlier, the query cache of another connection may instead be cleared, and the dirty query cache might be used by another worker.

This change makes sure that the ActiveRecord middleware cannot be used in tandem with the new Rails reloader (in case folks have added it to their middleware stacks explicitly) and removes it from the default middleware stack when the reloader is in play.

I couldn't figure out a way to raise the error from the middleware during startup. It seems the middleware is instantiated per job and so the raise only happens when a job is run? Not sure if there's a better way.

Fixes the query cache issue in #3157.

/cc @mperham @matthewd

The ActiveRecord middleware releases any current ActiveRecord connections before the reloader has a chance to clear their query caches. We can just remove this middleware because the reloader takes care of clearing active connections, too.

sj26 · 2016-09-28T05:25:53Z

The test failure looks like an unrelated jruby concurrency issue?

matthewd · 2016-09-28T05:22:52Z

+          # We need to make sure folks remove this middleware if they've added
+          # it themselves.
+          #
+          if defined?(Sidekiq::Rails::Reloader) && Sidekiq.options[:reloader].is_a?(Sidekiq::Rails::Reloader)


Might be simpler to just throw this condition around the clear_active_connections! call... not sure.

I did that initially but moved it here hoping that it might raise earlier in the Sidekiq boot, but sadly it only raises during a job run. Happy to move it back if it seems more sensible.

Moved it back.

matthewd · 2016-09-28T05:23:09Z

+          # it themselves.
+          #
+          if defined?(Sidekiq::Rails::Reloader) && Sidekiq.options[:reloader].is_a?(Sidekiq::Rails::Reloader)
+            raise ArgumentError, "Your are usign the Sidekiq ActiveRecord middleware and the new Rails 5 reloader which are incompatible. Please remove the ActiveRecord middleware from your Sidekiq middleware configuration."


Typos: Your, usign

matthewd · 2016-09-28T05:25:29Z

+          # cache data.
+          #
+          # We need to make sure folks remove this middleware if they've added
+          # it themselves.


Probably worth an explicit acknowledgement that this middleware is unnecessary because the reloader will do the job for us -- that it's not "incompatible so you're trading one feature for the other", it's "incompatible because duplication"

Yep, good call.

(I'd put it in the commit message but not in the comment!)

matthewd · 2016-09-28T05:26:54Z

      m.add Middleware::Server::Logging
      m.add Middleware::Server::RetryJobs
-      if defined?(::ActiveRecord::Base)
+      if defined?(::ActiveRecord::Base) && (!defined?(Sidekiq::Rails::Reloader) || Sidekiq.options[:reloader].is_a?(Sidekiq::Rails::Reloader))


Missing a !, I think?

Yep, despite my testing that slipped through!

I've combined the conditional to match the one in the middleware and negated the whole thing.

mperham · 2016-09-28T16:29:51Z

I think at boot time the code should either add the middleware to the default server-side list or activate the Reloader. Mutually exclusive.

sj26 · 2016-09-29T00:36:39Z

@mperham that's what we're doing?

We're also raising in the middleware in case somebody has explicitly added the ActiveRecord middleware themselves outside of the default middleware stack.

Although I've just realised that initialize has already happened by the time the reloader gets set, so if the user customises their middleware stack which lazily resolves the default server midddleware then it will have included the ActiveRecord middleware anyway.

There's a chicken and egg. We need to change initialization based on the presence of the reloader, but the reloader can't be present until we've initialized. I'll investigate.

mperham · 2016-09-29T00:54:45Z

Yep, all this logic should go in sidekiq/rails.rb or cli.rb.

On Sep 28, 2016, at 5:36 PM, Samuel Cochran notifications@github.com wrote:

@mperham that's what we're doing?

Although I've just realised that initialize has already happened by the time the reloader gets set, so if the user customises their middleware stack which lazily resolves the default server midddleware then it will have included the ActiveRecord middleware anyway.

There's a chicken and egg. We need to change initialization based on the presence of the reloader, but the reloader can't be present until we've initialized. I'll investigate.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

sj26 · 2016-09-29T06:38:47Z

How about this version using railtie hooks within sidekiq/rails.rb?

We can't add the middleware earlier because we need at least config/boot and really config/application to load so that bundler is setup and our defined?(ActiveRecord) is effective (i.e. after require "rails/all" or require "active_record/railtie"). But we need to make sure the default middleware is in place as soon as possible before user code which might change middleware has run.

The reloader needs to come quite late in case cache_classes is changed. We could probably pull the reloader into the same block as the middleware except cache_classes might change later which would miss the code reloading log entry.

This setup should allow users to change middleware in config/environments/* or config/initializers/* just fine. If they're changing it in config/application.rb then we're buggered.

mperham · 2016-09-29T14:35:31Z

Looks pretty good to me now. Is the runtime check in the middleware still necessary? That's constant overhead that the user will have to pay for every job executed.

Also clarify exactly when these hooks are run.

sj26 · 2016-09-30T02:35:28Z

I've changed the middleware hook to even earlier which hopefully might even cover cases where sidekiq configuration is performed in config/application.rb.

The runtime check in the middleware is a just-in-case for folks who might have explicitly added the ActiveRecord middleware themselves. If you think this is a remote enough chance then maybe we could drop it?

Oh, or we could move the check to when we setup the reloader and explode there if we detect the ActiveRecord middleware in the middleware chain. I'll draft that up.

sj26 · 2016-09-30T03:10:41Z

+        # The reloader also takes care of ActiveRecord but is incompatible with
+        # the ActiveRecord middleware so make sure it's not in the chain already.
+        if defined?(Sidekiq::Middleware::Server::ActiveRecord) && Sidekiq.server_middleware.exists?(Sidekiq::Middleware::Server::ActiveRecord)
+          raise ArgumentError, "You are using the Sidekiq ActiveRecord middleware and the new Rails 5 reloader which are incompatible. Please remove the ActiveRecord middleware from your Sidekiq middleware configuration."


I'm not sure if this should raise, Sidekiq.logger.warn, Rails.logger.warn, or ActiveSupport::Deprecation.warn. Deprecation feels the most rails-ey. Thoughts?

This executor currently relies on `ActiveRecord::Base.connection` not changing between `prepare` and `complete`. If something else returns the current ActiveRecord connection to the pool early then this `complete` call will fail to clear the correct query cache and restore the original `query_cache_enabled` status. This has for example been happening in Sidekiq: sidekiq/sidekiq#3166 We can just keep track of the connection as part of the exector state.

sj26 added 2 commits September 28, 2016 14:47

Prevent AR middleware and reloader clash

8a75409

The ActiveRecord middleware releases any current ActiveRecord connections before the reloader has a chance to clear their query caches. We can just remove this middleware because the reloader takes care of clearing active connections, too.

Make sure folks haven't added the middleware manually

dcd6619

sj26 mentioned this pull request Sep 28, 2016

Rails 5 Reloader Threading Issues #3157

Closed

matthewd reviewed Sep 28, 2016

View reviewed changes

sj26 added 4 commits September 28, 2016 20:29

Fix the conditional

e851544

Mention that the reloader also clears connections

0d9a89a

Move it all back into a single method

c9f7817

Typos

c6c623e

Pull all rails setup into railtie

603f7e3

Set the middleware even earlier

343206a

Also clarify exactly when these hooks are run.

Only check ActiveRecord reloader/middleware sanity at boot

38f6cfa

sj26 commented Sep 30, 2016

View reviewed changes

sj26 mentioned this pull request Sep 30, 2016

ActiveRecord Query Cache sometimes dirty and stuck enabled across threads rails/rails#26666

Closed

mperham merged commit 659dea9 into sidekiq:master Sep 30, 2016

sj26 deleted the reloader-and-activerecord-middleware branch November 20, 2024 23:41

Conversation

sj26 commented Sep 28, 2016

Uh oh!

sj26 commented Sep 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mperham commented Sep 28, 2016

Uh oh!

sj26 commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mperham commented Sep 29, 2016

Uh oh!

sj26 commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mperham commented Sep 29, 2016

Uh oh!

sj26 commented Sep 30, 2016

Uh oh!

sj26 Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sj26 commented Sep 29, 2016 •

edited

Loading

sj26 commented Sep 29, 2016 •

edited

Loading

sj26 Sep 30, 2016 •

edited

Loading