Skip to content

Move advisory locks to own connection handler. - #39758

Merged
eileencodes merged 1 commit into
rails:masterfrom
tgxworld:create_own_connection_handler_for_advisory_locks
Aug 4, 2020
Merged

Move advisory locks to own connection handler.#39758
eileencodes merged 1 commit into
rails:masterfrom
tgxworld:create_own_connection_handler_for_advisory_locks

Conversation

@tgxworld

@tgxworld tgxworld commented Jun 30, 2020

Copy link
Copy Markdown
Contributor

Summary

Resolves #39157

Removes the use of ActiveRecord::AdvisoryLockBase since it inherits
from ActiveRecord::Base and hence share module attributes that are defined in ActiveRecord::Base.
This is problematic because establishing connections through
ActiveRecord::AdvisoryLockBase can end up changing state of the default
connection handler of ActiveRecord::Base leading to unexpected
behaviors in a Rails application.

In the case of #39157,

Running migrations with rails db:migrate:primary_shard_one was not working as
the application itself defined the following

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  connects_to shards: {
    default: { writing: :primary },
    shard_one: { writing: :primary_shard_one }
  }
end

In the database migrate rake task, the default connection was
established with the database config of primary_shard_one. However,
the default connection was altered to that of primary because
ActiveRecord::AdvisoryLockBase.establish_connection ended up loading
ApplicationRecord which calls connects_to shards:. Since all we
really need here is just a normal database connection, we can avoid
accidentally altering the default connection handler state during the migration
by creating a custom connection handler used for retrieving a connection.

Other Information

Alternate take on #38235

@tgxworld

Copy link
Copy Markdown
Contributor Author

cc @eileencodes @seejohnrun

I didn't add a test for this because the bug has to be reproduced through the use of ActiveRecord::AdvisoryLockBase.

@eileencodes eileencodes self-assigned this Jun 30, 2020

@eileencodes eileencodes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing, I've been 100% focused on our Ruby upgrade at GitHub. Now that that's done I think this looks good, I just have one question about the memoization. Thanks for working on this 💖

Comment thread activerecord/lib/active_record/migration.rb Outdated
@eileencodes eileencodes added this to the 6.1.0 milestone Jul 31, 2020
@tgxworld
tgxworld force-pushed the create_own_connection_handler_for_advisory_locks branch from 688cf2a to b1dc282 Compare August 3, 2020 00:49
@tgxworld

tgxworld commented Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

@eileencodes I've updated the PR as per your comment. Thank you for taking the time to review this 👍

@eileencodes

eileencodes commented Aug 3, 2020

Copy link
Copy Markdown
Member

Postgres and mysql tests are failing with:

Failure:
  | MigrationTest#test_with_advisory_lock_raises_the_right_error_when_it_fails_to_release_lock [/rails/activerecord/test/cases/migration_test.rb:941]:
  | [ActiveRecord::ConcurrentMigrationError] exception expected, not
  | Class: <NoMethodError>
  | Message: <"undefined method `connection' for nil:NilClass">
  | ---Backtrace---
  | /rails/activerecord/test/cases/migration_test.rb:944:in `block (3 levels) in test_with_advisory_lock_raises_the_right_error_when_it_fails_to_release_lock'
  | /rails/activerecord/lib/active_record/migration.rb:1388:in `block in with_advisory_lock'
  | /rails/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:467:in `with_connection'
  | /rails/activerecord/lib/active_record/migration.rb:1384:in `with_advisory_lock'
  | /rails/activerecord/test/cases/migration_test.rb:943:in `block (2 levels) in test_with_advisory_lock_raises_the_right_error_when_it_fails_to_release_lock'
  | /rails/activesupport/lib/active_support/testing/stream.rb:11:in `silence_stream'
  | /rails/activerecord/test/cases/migration_test.rb:942:in `block in test_with_advisory_lock_raises_the_right_error_when_it_fails_to_release_lock'

It looks like connection.release_advisory_lock doesn't have the connection defined since it's defined in a with_connection block above.

Removes the use of `ActiveRecord::AdvisoryLockBase` since it inherits
from `ActiveRecord::Base` and hence share module attributes that are defined in `ActiveRecord::Base`.
This is problematic because establishing connections through
`ActiveRecord::AdvisoryLockBase` can end up changing state of the default
connection handler of `ActiveRecord::Base` leading to unexpected
behaviors in a Rails application.

In the case of rails#39157,

Running migrations with `rails db:migrate:primary_shard_one` was not working as
the application itself defined the following

```
class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true

  connects_to shards: {
    default: { writing: :primary },
    shard_one: { writing: :primary_shard_one }
  }
end
```

In the database migrate rake task, the default connection was
established with the database config of `primary_shard_one`. However,
the default connection was altered to that of `primary` because
`ActiveRecord::AdvisoryLockBase.establish_connection` ended up loading
`ApplicationRecord` which calls `connects_to shards:`. Since all we
really need here is just a normal database connection, we can avoid
accidentally altering the default connection handler state during the migration
by creating a custom connection handler used for retrieving a connection.
@tgxworld
tgxworld force-pushed the create_own_connection_handler_for_advisory_locks branch from b1dc282 to 45add34 Compare August 4, 2020 02:18
@tgxworld

tgxworld commented Aug 4, 2020

Copy link
Copy Markdown
Contributor Author

@eileencodes The test should be fixed now. Apologies for not checking.

@eileencodes
eileencodes merged commit 8de10f9 into rails:master Aug 4, 2020
@eileencodes

Copy link
Copy Markdown
Member

Thanks!

@tgxworld

tgxworld commented Aug 4, 2020

Copy link
Copy Markdown
Contributor Author

Thank you for reviewing too :)

@tgxworld
tgxworld deleted the create_own_connection_handler_for_advisory_locks branch August 13, 2020 01:13
eileencodes added a commit to eileencodes/rails that referenced this pull request Oct 12, 2022
In rails#38235 I moved advisory locks to their own named connections, then
in rails#39758 the advisory lock was left on Base.connection but then moved
it it's own connection handler. I believe with rails#45450 that this change
was made obsolete and can be returned to the prior behavior without
having to open an additional connection. The tests added pass and I also
tested this in my local demo to ensure that this is working correctly.

When I originally changed the behavior here Matthew noted that this
could be surprising for some setups that expect only one connection for
a running migration. I thought there was an issue related to this but I
can't find it.
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.

rails db:migrate:{namespace} does not work with shards defined in ApplicationRecord

2 participants