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 granular swapping for primary_abstract_class #45621

Conversation

eileencodes
Copy link
Member

In #45450 we stopped removing connections when establish_connection was
called and an identical connection existed in the pool. Unfortunately
this broke granular connection swapping for the primary_abstract_class
because the class stored in the pool was incorrect.

What was happening was connection for the writing role were getting
stored with the class ActiveRecord::Base and connections for the reading
role were getting ApplicationRecord (or whatever the app set as their
primary_abstract_class. If we made sure that the reading connections
also got ActiveRecord::Base that would break granular swapping for both
writing and reading because Active Record doesn't know whether you
wanted global swapping or granular swapping for prevent writes. This bug
only manifests on prevent writes because it's the only value we actually
lookup on the connection pool (because at the point we need it we don't
have access to self).

To fix this I've decided to update the class on the existing pool if it
doesn't match the owner_name passed. This is kind of gross I admit, but
it's safe because the way we find connections is on the
connection_name. The class is only stored so that the connection can
find it's current_preventing_writes value. This also required me to
move the ivar to an instance method for connection_class on the pool
because we don't want to cache the value and want to make sure we're
getting it from the pool config directly.

cc/ thanks to @jhawthorn for the idea on how to fix this and for reporting the issue

@jhawthorn
Copy link
Member

cc/ thanks to @jhawthorn for the idea on how to fix this and for reporting the issue

Credit to @ahoglund for the debugging that lead to the issue 😁

In rails#45450 we stopped removing connections when establish_connection was
called and an identical connection existed in the pool. Unfortunately
this broke granular connection swapping for the primary_abstract_class
because the class stored in the pool was incorrect.

What was happening was connection for the writing role were getting
stored with the class ActiveRecord::Base and connections for the reading
role were getting ApplicationRecord (or whatever the app set as their
`primary_abstract_class`. If we made sure that the reading connections
also got ActiveRecord::Base that would break granular swapping for both
writing and reading because Active Record doesn't know whether you
wanted global swapping or granular swapping for prevent writes. This bug
only manifests on prevent writes because it's the only value we actually
lookup on the connection pool (because at the point we need it we don't
have access to self).

To fix this I've decided to update the class on the existing pool if it
doesn't match the owner_name passed. This is kind of gross I admit, but
it's safe because the way we find connections is on the
`connection_name`. The class is only stored so that the connection can
find it's `current_preventing_writes` value. This also required me to
move the ivar to an instance method for connection_class on the pool
because we don't want to cache the value and want to make sure we're
getting it from the pool config directly.
@eileencodes eileencodes force-pushed the fix-granular-swapping-for-primary_abstract_class branch from 07a7913 to 741d029 Compare July 19, 2022 12:22
@eileencodes
Copy link
Member Author

I decided to add an extra guard for primary_class? because that's the only time we actually want to change the name. There's not much risk of this breaking anything without the primary class check, but I think it makes the code more explicit that this behavior is for handling primary class only.

@eileencodes eileencodes merged commit bb68040 into rails:main Jul 19, 2022
@eileencodes eileencodes deleted the fix-granular-swapping-for-primary_abstract_class branch July 19, 2022 13:54
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

2 participants