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

[5.6] Fix check on empty queue #23757

Merged
merged 1 commit into from
Mar 31, 2018
Merged

Conversation

pdavide
Copy link
Contributor

@pdavide pdavide commented Mar 30, 2018

When using phpredis, blpop returns an empty array if no elements are found in the list.
This can be easily deduced looking at this test.

Fix #23756.

When using phpredis, `blpop` returns an empty array if no elements are found in the list.
This can be easily deduced looking at [this test](https://github.com/phpredis/phpredis/blob/300c72510c48e210338826b713f260a4eda8abc7/tests/RedisTest.php#L833).

Fix laravel#23756.
@pdavide pdavide changed the title Fixed check on empty queue [5.6] Fixed check on empty queue Mar 30, 2018
@pdavide pdavide changed the title [5.6] Fixed check on empty queue [5.6] Fix check on empty queue Mar 30, 2018
@tillkruss
Copy link
Contributor

tillkruss commented Mar 31, 2018

I'd rather see the PhpRedisConnection fixed, so we can rely on the return values across drivers.

@pdavide
Copy link
Contributor Author

pdavide commented Mar 31, 2018

Well, I think my simple solution will ensure compatibility across the drivers, as empty return true with both NULL and Array().
Additionally this little bug cause the application log file to grow fast and become unreadable because the queue worker is calling the blockingPop function every time it searches for an available job and the $rawBody[1] expression will raise an error when $rawbody is an empty array.

@tillkruss
Copy link
Contributor

@pdavide: It's only solved in RedisQueue, nowhere else. See #23761.

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