Skip to content

Commit

Permalink
Deprecate Redis.current
Browse files Browse the repository at this point in the history
  • Loading branch information
byroot committed Jan 20, 2022
1 parent bc7b0a2 commit 9745e22
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 5 deletions.
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Unreleased

* Fix `zpopmax` and `zpopmin` when called inside a pipeline. See #1055.
* Deprecate `Redis.current`.
* Deprecate calling commands on `Redis` inside `Redis#pipelined`. See #1059.
```ruby
redis.pipelined do
Expand All @@ -16,6 +16,9 @@
end
```
* Deprecate `Redis#queue` and `Redis#commit`. See #1059.

* Fix `zpopmax` and `zpopmin` when called inside a pipeline. See #1055.

* Add `Redis.silence_deprecations=` to turn off deprecation warnings.
If you don't wish to see warnings yet, you can set `Redis.silence_deprecations = false`.
It is however heavily recommended to fix them instead when possible.
Expand Down
12 changes: 8 additions & 4 deletions lib/redis.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,15 @@ def deprecate!(message)
::Kernel.warn(message) unless silence_deprecations
end

attr_writer :current
end
def current
deprecate!("`Redis.current=` is deprecated and will be removed in 5.0. (called from: #{caller(1, 1).first})")
@current ||= Redis.new
end

def self.current
@current ||= Redis.new
def current=(redis)
deprecate!("`Redis.current=` is deprecated and will be removed in 5.0. (called from: #{caller(1, 1).first})")
@current = redis
end
end

include Commands
Expand Down

3 comments on commit 9745e22

@NobodysNightmare
Copy link

@NobodysNightmare NobodysNightmare commented on 9745e22 Feb 3, 2022

Choose a reason for hiding this comment

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

Out of curiosity, is there any rationale behind deprecating Redis.current? Having exactly one redis client per application was such a common use case, that people would have either used a global variable ($redis) or if they knew Redis.current existed use this one.

Do you recommend building an own place to store the redis client if needed or any other alternative?

edit: For context, I've noticed the deprecation through a comment on https://stackoverflow.com/a/34673035/968531 and was wondering whether any general advice can be given

@byroot
Copy link
Collaborator Author

@byroot byroot commented on 9745e22 Feb 3, 2022

Choose a reason for hiding this comment

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

Because multi-threaded environments are very much the default these days, and sharing the same Redis instance between threads leads to tons of locking.

So I prefer not to encourage this anymore. As you said it's super easy to just use $redis instead if that's really what you want.

It's not the role of a database client to manage the lifecycle to the connection, it's up to the application to do that.

Do you recommend building an own place to store the redis client if needed

Yes. And you likely want to a connection pool with it.

@NobodysNightmare
Copy link

Choose a reason for hiding this comment

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

Thanks for the quick response and pointing to the locking issue.

Please sign in to comment.