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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Implement ConnectionPool#then #138

Merged
merged 1 commit into from Dec 1, 2020
Merged

Conversation

ianks
Copy link
Contributor

@ianks ianks commented Dec 1, 2020

What?

In Ruby 2.5, Object#then was added as a standard way to "yield self" to the caller. This convention is useful since it allows for library maintainers to abstract access to a value.

For example, Concurrent#Promise implements .then in a way which allow access to a concurrently resolved value:

Concurrent::Promise.execute { :foo }.then { |val| puts val } #=> :foo 

# provides the same value as

:foo.then { |val| puts val } #=> :foo 

The Problem

When using connection-pool in the wild, on multiple occasions I have been unable to directly pass a ConnectionPool wrapped Redis to a library. Most recently, we can't use a ConnectionPool Redis in rack-mini-profiler since it, rightfully, assumes the Redis connection is an actual Redis::Client.

Unfortunately, currently library maintainers like rack-mini-profiler have no easy way to support both raw Redis::Client and a ConnectionPool Redis. In order to be compatible with both, they would have to specifically check if the object responds to .with, and add conditional codepaths. Some library actually do this, which is nice! But overall, it's painful to have to support two codepaths.

Proposed Solution

By adding .then, gems would have a way to support both raw Redis and CP Redis using the same codepath, no conditionals. Offering this API offers an easily adoptable and ergonomic interface for Ruby 2.5+:

# only compatible with Redis::Client, and ConnectionPool::Wrapper
redis.set 'foo' 'bar'

# compatible with Redis::Client, and ConnectionPool Redis 馃帀
redis.then { |r| r.set 'foo' 'bar' }

Thoughts?

@mperham mperham merged commit 9105e96 into mperham:master Dec 1, 2020
@mperham
Copy link
Owner

mperham commented Dec 1, 2020

I will say, this kind of breaks then as we aren't yielding self, we are yielding a connection within the pool. Is this any reason for concern?

@mperham
Copy link
Owner

mperham commented Dec 1, 2020

BTW good writeup, Ian. Appreciate the explanation, makes it easy to make a quick (hasty?) decision. 馃槑

@ianks
Copy link
Contributor Author

ianks commented Dec 3, 2020

I will say, this kind of breaks then as we aren't yielding self, we are yielding a connection within the pool. Is this any reason for concern?

I would not think so. From a developer standpoint, when interacting with connection pool you really want the underlying resource, not the pool typically. And it is more useful.

mperham added a commit that referenced this pull request Dec 3, 2020
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.

None yet

2 participants