Skip to content
This repository was archived by the owner on Feb 7, 2024. It is now read-only.

[2.x] Async Redis queue driver #552

Merged
merged 11 commits into from
Sep 26, 2020
Merged

[2.x] Async Redis queue driver #552

merged 11 commits into from
Sep 26, 2020

Conversation

rennokki
Copy link
Collaborator

@rennokki rennokki commented Sep 25, 2020

Closes #190

Description

This PR adds a new async-redis connection that should be used in case you use Redis Queue features within the WebSockets server. It can also be set to default, because when no PHPReact Redis instance is available, it uses the default Redis connection.

Tests

Tests were taken from the following files, to ensure compatibility with the default Laravel's Redis queue/jobs tests:

Docs

Async Redis Queue

The default Redis connection also interacts with the queues. Since you might want to dispatch jobs on Redis from the server, you can encounter an anti-pattern of using a blocking I/O connection (like PhpRedis or PRedis) within the WebSockets server.

To solve this issue, you can configure the built-in queue driver that uses the Async Redis connection when it's possible, like within the WebSockets server. It's highly recommended to switch your queue to it if you are going to use the queues within the server controllers, for example.

Add the async-redis queue driver to your list of connections. The configuration parameters are compatible with the default redis driver:

'connections' => [
    'async-redis' => [
        'driver' => 'async-redis',
        'connection' => env('WEBSOCKETS_REDIS_REPLICATION_CONNECTION', 'default'),
        'queue' => env('REDIS_QUEUE', 'default'),
        'retry_after' => 90,
        'block_for' => null,
    ],
]

Also, make sure that the default queue driver is set to async-redis:

QUEUE_CONNECTION=async-redis

@rennokki rennokki changed the base branch from master to 2.x September 25, 2020 19:17
@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2020

Codecov Report

Merging #552 into 2.x will decrease coverage by 0.33%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##              2.x     #552      +/-   ##
==========================================
- Coverage   89.58%   89.24%   -0.34%     
==========================================
  Files          52       54       +2     
  Lines        1478     1497      +19     
==========================================
+ Hits         1324     1336      +12     
- Misses        154      161       +7     
Impacted Files Coverage Δ
src/Queue/AsyncRedisConnector.php 0.00% <0.00%> (ø)
src/WebSocketsServiceProvider.php 92.59% <83.33%> (-1.41%) ⬇️
src/ChannelManagers/RedisChannelManager.php 97.18% <100.00%> (+0.02%) ⬆️
src/Queue/AsyncRedisQueue.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40ee5fb...d0b4f46. Read the comment docs.

@francislavoie
Copy link
Contributor

Seems alright to me.

I think changes were made to the RedisQueue implementation since I last looked at it (Laravel 5.6 ish) such that it now just fires off the redis command and doesn't care about the result. I thought more work would need to be done to handle promises and such.

Seems alright though, as long as basically just push and later are used to publish jobs.

@rennokki
Copy link
Collaborator Author

rennokki commented Sep 26, 2020

Yes, I have checked the inherited queue class and it seems like all it does is to push it to the queue, without caring too much about the result. So as long as the tests check if the zadd is called with the proper job details, it should work as expected.

Either way, when the channel manager is bind to the container it would use the React's Redis client (like in the Server command process), otherwise will just fallback to the default parent connection (like PhpRedis or Predis)

@rennokki rennokki merged commit 9cb83dc into 2.x Sep 26, 2020
@rennokki rennokki deleted the feature/async-queue branch September 26, 2020 12:37
@deantheiceman
Copy link

@rennokki Could you clarify in the docs is it everything which should use async-redis ?

If so, laravel horizon is no longer compatitable:

local.ERROR: Call to undefined method BeyondCode\LaravelWebSockets\Queue\AsyncRedisQueue::readyNow() {"exception":"[object] (Error(code: 0): Call to undefined method BeyondCode\\LaravelWebSockets\\Queue\\AsyncRedisQueue::readyNow() at /var/www/html/vendor/laravel/horizon/src/AutoScaler.php:80

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants