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

Simplify adapter construction; defer connect until first use #44591

Merged
merged 8 commits into from
Jul 29, 2022

Conversation

matthewd
Copy link
Member

@matthewd matthewd commented Mar 1, 2022

Building on from #44576, we no longer need to connect to the database in order to construct an adapter instance: we instead supply just the config hash, and it will connect on demand, in the same way it already knows to handle later reconnections.

This cleans up a weird API overlap, where we previously needed to know how to connect in two different places.

@matthewd matthewd force-pushed the defer-db-connect branch 4 times, most recently from 07d39b2 to f534af2 Compare March 2, 2022 15:10
@matthewd matthewd force-pushed the defer-db-verify branch 2 times, most recently from 4a45007 to e02e06e Compare March 8, 2022 15:45
@matthewd matthewd force-pushed the defer-db-verify branch 2 times, most recently from 910340d to 8ff11be Compare March 23, 2022 13:07
@logger = ActiveRecord::Base.logger

if deprecated_logger || deprecated_connection_options || deprecated_config
raise ArgumentError, "new API accepts just one config hash"
Copy link
Member

Choose a reason for hiding this comment

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

This is a confusing error because if I passed a config hash and a logger I wouldn't know what this error was indicating I should change. I know this is a WIP but wanted to note I think each deprecated option should have it's own error to be clearer about the new API.

if config_or_deprecated_connection.is_a?(Hash)
@raw_connection = nil
@config = config_or_deprecated_connection.symbolize_keys
@logger = ActiveRecord::Base.logger
Copy link
Member

Choose a reason for hiding this comment

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

Are we saying adapters can't set their own logger now? Do any adapters set their own logger? I think that'd be odd but not impossible.

Copy link
Member Author

Choose a reason for hiding this comment

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

My plan is to ultimately pull the connection's base model from the handler (or pass it in the supplied config hash?), and then use that object's logger -- so you can't directly override the adapter's logger during initialize (which hardly anyone calls anyway -- note that mysql2_connection and friends are already just passing logger), but there is some control beyond the hard-coded AR::Base.

nil,
config,
)
ConnectionAdapters::Mysql2Adapter.new(config)
Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like this cleanup.

end

def reconnect!(restore_transactions: false)
@lock.synchronize do
@raw_connection.close
@raw_connection&.close
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that now we have to keep checking for nil everywhere. I'm concerned we'll forget to do this in the future for a new method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's fairly fine... we have plenty of potentially-nil ivars in various places. The main mitigating factor here is that we should rarely add new methods that use the variable directly: outside of connection management (dis/re-/connect), nearly everything else should use with_raw_connection, valid_raw_connection, or any_raw_connection (in that preference order).

The other side is just that there isn't much of an alternative: while abstract adapter defines that @raw_connection exists, is nil when not connected, and truthy when connected... the rest of the API is adapter-specific, so it's impossible for us to use a null-object pattern, say.

@connection.disconnect!

assert_raise(ActiveRecord::ConnectionNotEstablished) do
@connection.quote("string")
end
Copy link
Member

Choose a reason for hiding this comment

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

Could this potentially be an undesirable change in behavior for applications?

Copy link
Member Author

Choose a reason for hiding this comment

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

Theoretically, yes -- I believe this is the one observable difference in existing API behaviour, outside of timing.

I did consider separately tracking a concept of explicit disconnection, and having that persist until an equally-explicit reconnect... but I just can't come up with a reasonable use case we're breaking to justify the extra code / state. The obvious use for disconnect! would be that you e.g. want to restart the DB server, so need to break your existing session... but we still do that; we just reconnect on demand instead of leaving the adapter in a semi-permanent state of brokenness.

adrianna-chang-shopify added a commit to adrianna-chang-shopify/rails that referenced this pull request Sep 21, 2022
Building on the work done in rails#44576 and rails#44591, we extend the logic that automatically
reconnects broken db connections to take into account a timeout limit. This ensures
that retries + reconnects are slow-query aware, and that we don't retry queries
if a given amount of time has already passed since the query was first tried.

This value will default to 5 seconds, but can be adjusted via the `connection_retry_timeout`
config.
adrianna-chang-shopify added a commit to adrianna-chang-shopify/rails that referenced this pull request Sep 22, 2022
Building on the work done in rails#44576 and rails#44591, we extend the logic that automatically
reconnects broken db connections to take into account a timeout limit. This ensures
that retries + reconnects are slow-query aware, and that we don't retry queries
if a given amount of time has already passed since the query was first tried.

This value will default to 5 seconds, but can be adjusted via the `connection_retry_timeout`
config.
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request May 8, 2023
yahonda added a commit to rsim/oracle-enhanced that referenced this pull request May 8, 2023
Make sure unit tests run successfully before rails/rails#44591 is merged
davinlagerroos pushed a commit to umn-asr/oracle-enhanced that referenced this pull request Feb 14, 2024
We can't call @raw_connection right away anymore, because it is nil until
it is used after the changes in rails/rails#44576
and rails/rails#44591.

Also we need to have @lock established in order to be able to use
`with_raw_connection`. In the abstract_adapter implementation, that happens
after the arel_visitor is set, but in oracle enhanced, we need to use
`with_raw_connection` to check the database version in order to pick with
visitor to use. Calling lock_thread = nil before super means @lock isi
available to set the arel_visitor.

Rails rails/rails#48188 also requires an internal_exec_query
method that is called by the abstract adpater's exec_query. Naively renaming
the oracle-enhanced's exec_query to internal_exec_query makes the oracle
enhanced tests run.

I also saw errors due to `active?` using @raw_connection. `valid_raw_connection`
ends up calling `active?` via `with_raw_connection` and `verify` so that
can't be used as a replacement for @raw_connection here. Borrowing from
the postgres adapter implementation to get around this.
davinlagerroos pushed a commit to umn-asr/oracle-enhanced that referenced this pull request Feb 14, 2024
We can't call @raw_connection right away anymore, because it is nil until
it is used after the changes in rails/rails#44576
and rails/rails#44591.

Also we need to have @lock established in order to be able to use
`with_raw_connection`. In the abstract_adapter implementation, that happens
after the arel_visitor is set, but in oracle enhanced, we need to use
`with_raw_connection` to check the database version in order to pick with
visitor to use. Calling lock_thread = nil before super means @lock isi
available to set the arel_visitor.

Rails rails/rails#48229 also requires an internal_exec_query
method that is called by the abstract adpater's exec_query. Naively renaming
the oracle-enhanced's exec_query to internal_exec_query makes the oracle
enhanced tests run.

I also saw errors due to `active?` using @raw_connection. `valid_raw_connection`
ends up calling `active?` via `with_raw_connection` and `verify` so that
can't be used as a replacement for @raw_connection here. Borrowing from
the postgres adapter implementation to get around this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants