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

[10.x] Disable PhpRedis serialization & compression for RedisQueue #47531

Closed
wants to merge 3 commits into from
Closed

[10.x] Disable PhpRedis serialization & compression for RedisQueue #47531

wants to merge 3 commits into from

Conversation

Huggyduggy
Copy link

This PR addresses issue #47356 : PhpRedis Serialization & compression within queues messes with Lua scripts. As a result, event listeners & jobs are being pushed to :reserved queues, even when finished successfully, filling up the Redis memory in no-time.

This PR disables compression & serialization for Redis queues, without losing the benefits of compression & igbinary/msgpack serialization for caching purposes. It checks if the current Redis connection is handled by PhpRedis & if the Redis extension is installed (thus not affecting Predis).

As of the coding style:

  1. I've figured the RedisQueue constructor would be the best fit, even though I usually avoid calling methods within constructors. It could also be added to the QueueServiceProvider.

  2. I'm not too sure if checking for PhpRedis is really necessary, as - as far as I can see - Predis would simply ignore those settings. Yet, at least for better understanding of the code, I've added the check. Checking for constants is added to avoid hickups if Redis extension is not installed, thus no constants would be available.

Feedback is greatly appreciated!

@Huggyduggy Huggyduggy marked this pull request as draft June 22, 2023 07:08
@Huggyduggy Huggyduggy marked this pull request as ready for review June 22, 2023 13:23
@Huggyduggy
Copy link
Author

I'm having quite a hard time understanding why mockery fails - may I ask anyone with more mockery-experience to chip in?

@mfn
Copy link
Contributor

mfn commented Jun 22, 2023

You are calling getConnection() already when constructing the queue. The previous code didn't do this.

Insofar, when you look at the tests like this \Illuminate\Tests\Queue\QueueRedisQueueTest::testPushProperlyPushesJobOntoRedis

$queue = $this->getMockBuilder(RedisQueue::class)->onlyMethods(['getRandomId'])->setConstructorArgs([$redis = m::mock(Factory::class), 'default'])->getMock();

You will see that it adds a mock of the redis factory to the constructor.

But this mock has not yet any expectations set. They are set later.


And this actually makes me disagree with your approach: upon creating the RedisQueue, a working connection already will be established, whereas before it's done "on demand" when needed.

@taylorotwell taylorotwell marked this pull request as draft June 23, 2023 15:43
@driesvints
Copy link
Member

@Huggyduggy are you still working on this?

@Huggyduggy
Copy link
Author

Hi Dries,

I'd keep the PR open for the moment - I'm still trying to find better places within the code for this issue to be fixed, but I haven't found one yet. Either we're moving closer to the RedisManager or Connection, which most likely leads to issues when the same connection is used for queue & caching (e.g. serialization & compression is unset too often); or we're moving to the LuaTemplates, which would be triggered too often.

@driesvints
Copy link
Member

@Huggyduggy feel free to resend this once you have time to finish it. Thanks.

@driesvints driesvints closed this Sep 1, 2023
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.

3 participants