Skip to content

Patch ConnectionPool to carry semian_resource #575

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

Closed
wants to merge 1 commit into from
Closed

Conversation

kirs
Copy link
Contributor

@kirs kirs commented Apr 4, 2025

I've been working on replacing references to a single global Redis connection in one of the Shopify apps with a ConnectionPool-wrapped Redis to make it thread-safe.

I found that so many spots expect to be able to call semian_resource on that connection object. And I don't think we should require those to go check out connection from a pool via @redis_pool.with { conn.semian_resource } to access semian_resource.

Instead, we could add semian_resource as an accessor on ConnectionPool for convenience.

Example of using that with Redis:

semian_resource = ::Redis.new(...).semian_resource
ConnectionPool.new(size: 5, timeout: 0, semian_resource: semian_resource) do
  ::Redis.new(...)
end

cc @byroot @danmayer for review

@casperisfine
Copy link
Contributor

Any reason why you wouldn't just subclass/decorate ConnectionPool in your private code?

@kirs
Copy link
Contributor Author

kirs commented Apr 7, 2025

That's a great idea.

@kirs kirs closed this Apr 7, 2025
@kirs kirs deleted the conn-pool branch April 7, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants