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

Fix deprecation warning in ActiveRecord 7.1 when clearing active connections #498

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

haffla
Copy link

@haffla haffla commented May 6, 2024

Hi. Passing no argument to ActiveRecord::Base.connection_handler.clear_active_connections! was deprecated here and produces following log.

DEPRECATION WARNING: `clear_active_connections!` currently only applies to connection pools in the current role (`writing`). In Rails 7.2, this method will apply to all known pools, regardless of role. To affect only those connections belonging to a specific role, pass the role name as an argument. To switch to the new behavior, pass `:all` as the role name.

Rails documentation of method: https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/ConnectionHandler.html#method-i-clear_active_connections-21

@haffla
Copy link
Author

haffla commented May 6, 2024

Failing Github actions don't seem to be related to my change.

@lancetarn
Copy link
Contributor

Hello @haffla ! Thanks for putting this up. We'll have to see what changed out from under us to cause those test failures.

@haffla
Copy link
Author

haffla commented May 21, 2024

Hi @lancetarn. Were you able to dig into this?

@haffla
Copy link
Author

haffla commented May 21, 2024

The issue seems to be

LoadError: Error loading the 'sqlite3' Active Record adapter. Missing a gem it depends on? can't activate sqlite3 (~> 1.4), already activated sqlite3-2.0.1. Make sure all dependencies are added to Gemfile.

Rails seems to have had the same issue.

@haffla
Copy link
Author

haffla commented Jul 16, 2024

@lancetarn all checks passed now

@@ -51,7 +51,7 @@ def data
ensure
# Sometimes :database_engine and :database_adapter can cause a reference to an AR connection.
# Make sure we release all AR connections held by this thread.
ActiveRecord::Base.connection_handler.clear_active_connections! if Utils::KlassHelper.defined?("ActiveRecord::Base")
ActiveRecord::Base.connection_handler.clear_active_connections!(:all) if Utils::KlassHelper.defined?("ActiveRecord::Base")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello - I wanted to open the same PR (when running tests this deprecation warning gets quite verbose). In any case I think this is a safer way to implement this (backwards compatibility):

      if ActiveRecord::Base.connection_handler.respond_to?(:clear_active_connections!)
        ActiveRecord::Base.connection_handler.clear_active_connections!(:all)
      elsif ActiveRecord::Base.respond_to?(:clear_active_connections!)
        # ~> Rails 3.2 -> https://apidock.com/rails/ActiveRecord/Base/clear_active_connections%21/class
        ActiveRecord::Base.clear_active_connections!
      end

Copy link
Author

@haffla haffla Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marianposaceanu the link you posted: https://apidock.com/rails/ActiveRecord/Base/clear_active_connections%21/class actually shows that ActiveRecord::Base#clear_active_connections! just calls connection_handler.clear_active_connections!. So isn't it really the same?

Also there is a test for Rails 3 here and it's green.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you mean that clear_active_connections! doesn't take any arguments in Rails 3? That could be actually. But I wonder why no test is failing. I am gonna look into it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh lol, this code isn't even tested?!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so I think before touching this part, we would have to write some unit tests.

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

Successfully merging this pull request may close these issues.

3 participants