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

Add missing super in prune_thread_cache method #51085

Merged
merged 1 commit into from Feb 14, 2024

Conversation

eugeneius
Copy link
Member

@eugeneius eugeneius commented Feb 14, 2024

Followup to #50938.

QueryCache::ConnectionPoolConfiguration is prepended to ConnectionPool and its prune_thread_cache method doesn't call super, so this method was never called. It's also unnecessary: reaped connections are already removed from @thread_cached_conns by the remove_connection_from_thread_cache call in steal!.

@byroot
Copy link
Member

byroot commented Feb 14, 2024

It's also unnecessary: reaped connections are already removed from @thread_cached_conns by the remove_connection_from_thread_cache call in steal!

That's no longer fully true though, not in testing at least because of the connection pinning. Now when you are using transactional fixtures, the same connection my be associated to multiple threads, but still only has one official owner, hence this pruning.

The missing super is definitely a mistake, and it probably had limited consequence because that's an edge case that only happens in test but we should fix it, not remove the whole thing.

@eugeneius
Copy link
Member Author

I originally just added the missing call to super, but then tried to write a test for it and couldn't see a case where it would be needed. Thanks for the pointer to #50999, I see now since checkout returns the pinned connection it happens here:

def connection
@thread_cached_conns[ActiveSupport::IsolatedExecutionState.context] ||= checkout
end

I'll just add the call to super, sorry for second guessing you!

@byroot
Copy link
Member

byroot commented Feb 14, 2024

sorry for second guessing you!

Absolutely not, I make as many mistake as the next person.

@eugeneius eugeneius changed the title Remove unnecessary prune_thread_cache method Add missing super in prune_thread_cache method Feb 14, 2024
@eugeneius
Copy link
Member Author

As demonstrated here, so do I 😄

@eugeneius eugeneius merged commit 3163bb7 into rails:main Feb 14, 2024
4 checks passed
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