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

Move while_preventing_writes from conn to handler #36469

Merged

Conversation

eileencodes
Copy link
Member

If we put the while_preventing_writes on the connection then the
middleware that sends reads to the primary and ensures they can't write
will not work. The while_preventing_writes will only be applied to the
connection which it's called on - which in the case of the middleware is
Ar::Base.

This worked fine if you called it directly like
OtherDbConn.connection.while_preventing_writes but Rails didn't have a
way of knowing you wanted to call it on all the connections.

The change here moves the while_preventing_writes method from the
connection to the handler so that it can block writes to all queries for
that handler. This will apply to all the connections associated with
that handler.

cc/ @matthewd @tenderlove @jhawthorn @rafaelfranca

yield
ensure
@prevent_writes = original
replica? || ActiveRecord::Base.connection_handler.prevent_writes
Copy link
Member

Choose a reason for hiding this comment

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

I never remember this but, does each model have its different connection handler or all models always share the ActiveRecord::Base connection handler?

Copy link
Member Author

Choose a reason for hiding this comment

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

Each type of connection has it's own handler depending on what you put in connects_to.

Given the following:

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

  connects_to database: { writing: :primary, reading: :primary_replica }
end

class Person < ApplicationRecord
end

class AnimalsBase < ApplicationRecord
  self.abstract_class = true

  connects_to database: { writing: :animals, reading: :animals_replica }
end

class Dog < AnimalsBase
end

This will create 2 handlers a :writing handler and a :reading handler (aka role). Each of these handlers has 2 connections: one for application record (person) and one for animals base (dog). Because we use connected_to to swap the handler out:

ActiveRecord::Base.connected_to(role: :reading) do
  # do something on reading handler
end

means that ActiveRecord::Base.connection_handler.prevent_writes will ask the handler whether it should prevent writes instead of the connection. Since connected_to operates on the handler we can be confident it's passing the value down correctly.

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 added a test that demonstrates this is working correctly here https://github.com/rails/rails/pull/36469/files#diff-3ad790de5c6c5fbd6d956e443bcc0490R1579

@@ -986,15 +986,30 @@ def self.discard_unowned_pools(pid_map) # :nodoc:
end
end

attr_accessor :prevent_writes
Copy link
Member

Choose a reason for hiding this comment

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

We don't need public attr_writer :prevent_writes.

Suggested change
attr_accessor :prevent_writes
attr_reader :prevent_writes

@eileencodes eileencodes force-pushed the move-while_preventing_writes-to-handler branch 3 times, most recently from 0656798 to f53f9b2 Compare June 14, 2019 13:39
If we put the `while_preventing_writes` on the connection then the
middleware that sends reads to the primary and ensures they can't write
will not work. The `while_preventing_writes` will only be applied to the
connection which it's called on - which in the case of the middleware is
Ar::Base.

This worked fine if you called it directly like
`OtherDbConn.connection.while_preventing_writes` but Rails didn't have a
way of knowing you wanted to call it on all the connections.

The change here moves the `while_preventing_writes` method from the
connection to the handler so that it can block writes to all queries for
that handler. This will apply to all the connections associated with
that handler.
@eileencodes eileencodes force-pushed the move-while_preventing_writes-to-handler branch from f53f9b2 to cd881ab Compare June 14, 2019 20:11
@eileencodes eileencodes merged commit 3ac50c0 into rails:master Jun 14, 2019
@eileencodes eileencodes deleted the move-while_preventing_writes-to-handler branch June 14, 2019 21:14
eileencodes added a commit that referenced this pull request Jun 14, 2019
…es-to-handler

Move while_preventing_writes from conn to handler
suketa added a commit to suketa/rails_sandbox that referenced this pull request Oct 26, 2019
Move while_preventing_writes from conn to handler
rails/rails#36469
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.

None yet

3 participants