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

Since rails 7.1, executing with a disconnected Mysql2Adapter no longer raises ConnectionNotEstablished. #49733

Closed
fumihumi opened this issue Oct 21, 2023 · 2 comments

Comments

@fumihumi
Copy link

Since rails 7.1, executing with a disconnected Mysql2Adapter no longer raises ConnectionNotEstablished.

Steps to reproduce

in my project, conn.disconnect! is used to disconnect the connection after detecting a temporary database connection failure.
(e.g. failover / connection failure due to restart...etc)

Until Rails 7.0.8, when using a disconnected connection, ActiveRecord::ConnectionNotEstablished would be raised and conn.execute would fail.
(I have a mechanism to try to reconnect using this ConnectionNotEstablished Error)

Since Rails 7.1.0, conn.execute does not raise an error despite being disconnected.

Perhaps this change was made by this PR ( #44591).

Are there any plans to modify it to raise the error?

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"
  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", '7.1.1' # failed
  # gem "rails", '7.1.0' # failed
  # gem "rails", '7.0.8' # passed

  gem "mysql2"
end

require "active_record"
require "minitest/autorun"
require "logger"

db_name = "test_connection"
options = { host: "127.0.0.1", port: 3306 }

adapter = 'mysql2'

ActiveRecord::Base.establish_connection(adapter: adapter, username: "root", **options)
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Base.connection.drop_database db_name
ActiveRecord::Base.connection.create_database db_name
ActiveRecord::Base.establish_connection(adapter: adapter, username: "root", database: db_name, **options)
ActiveRecord::Schema.define do
  create_table :users, force: true do |t|
    t.string :name
  end
end

class User < ActiveRecord::Base; end

class BugTest < Minitest::Test
  def test_disconnected_conn_execute
    conn = User.connection
    conn.disconnect!
     # v7.0.8: pass, v7.1.0 and v7.1.1: failed
    assert_raises(ActiveRecord::ConnectionNotEstablished) { conn.execute("select 1;") }
  end
end

Expected behavior

ConnectionNotEstablished is raised in case of execute using a disconnected connection.

Actual behavior

The execute method does not raise an error.
execute completes successfully and returns Mysql::Result.

System configuration

Rails version: 7.1.1
Ruby version: 3.2.2

@matthewd
Copy link
Member

Explicit disconnect! is an edge case, but generally I think this is expected behaviour -- if we aren't connected, we're trying to do something that requires us to be connected, and we know how to connect... it seems reasonable that we'd do the thing that should work.

We could specifically track the fact that we've manually disconnected and therefore should not auto-reconnect, but it mostly just seems like avoidable complication.

I'm open to being convinced, though.. is there a compelling use for being able to leave the connection broken? (You describe error recovery & reconnection code in your app -- does Active Record's new automatic reconnection behaviour not work for you?)

@byroot
Copy link
Member

byroot commented Oct 28, 2023

I agree with @matthewd that this sounds more like a "bug fix / improvements" than a regression.

So I'll close but we can re-open if you still consider it a bug and make a case for it.

@byroot byroot closed this as completed Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants