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

Adds the ability to force a new connection to Redis #18830

Merged
merged 1 commit into from
Apr 17, 2017
Merged

Adds the ability to force a new connection to Redis #18830

merged 1 commit into from
Apr 17, 2017

Conversation

markhuot
Copy link
Contributor

When using Redis' SUBSCRIBE the recommendation is to open a second connection for any key modification you need to do. E.g. Laravel does not currently allow you to modify keys why subscribed, causing the following to error,

Redis::subscribe('queue-notifications', function ($msg) {
  $job = Redis::rpoplpush('queue-jobs', 'working-jobs');
});

The exact error is,

ERR only (P)SUBSCRIBE / (P)UNSUBSCRIBE / PING / QUIT allowed in this context

This PR allows you to work around this by forcing a new connection to Redis, when necessary,

Redis::subscribe('queue-notifications', function ($msg) {
  $redis = Redis::connection(null, true);
  $job = $redis->rpoplpush('queue-jobs', 'working-jobs');
});

The need for a second connection is also stated here,

http://stackoverflow.com/questions/22668244/should-i-use-separate-connections-for-pub-and-sub-with-redis

Note, this could probably be more expressive with Redis::newConnection() but I wanted to start here.

@taylorotwell
Copy link
Member

Sounds kinda more intuitive to have a disconnect() method like DB?

@markhuot
Copy link
Contributor Author

markhuot commented Apr 17, 2017

I don't want to disconnect either connection. I actually need both connections to be active so I can continue to listen on one connection while interacting with Redis over the second connection.

Some additional reports of similar needs on other frameworks,

@taylorotwell taylorotwell merged commit 940f43c into laravel:5.4 Apr 17, 2017
@taylorotwell
Copy link
Member

Easier for me to just make resolve public. I can't change the interface on a point release.

@markhuot
Copy link
Contributor Author

👍 makes sense. Thank you.

Would you like another PR based on master that adds Redis::newConnection($name = null) for a later release? Or, is resolve the way to go for this?

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