Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Fix redis Connection::get() should be non blocking by default#31545
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
src/Symfony/Component/Messenger/Transport/RedisExt/Connection.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
a64ef8e to454a738Comparechalasr commentedMay 19, 2019
ping@alexander-schranz also :) |
705b6f8 to81136e7Comparesrc/Symfony/Component/Messenger/Transport/RedisExt/Connection.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Transport/RedisExt/Connection.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
alexander-schranz left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
xReadGroup should be non blocking aslong as we don't give a block time. Did I implement there something false?
And as written the xlen I think is the false way to get the correct count value as this does also include pending and received messages.
81136e7 tof478351Comparealexander-schranz left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As discussed on slack this seems currently the only way to avoid a blocking request to omit the blocking parameter ofxreadgroup function.
@chalasr thank you for fixing this 👍
f478351 to229502aComparefabpot commentedMay 21, 2019
Thank you@chalasr. |
…king by default (chalasr)This PR was merged into the 4.3 branch.Discussion----------[Messenger] Fix redis Connection::get() should be non blocking by default| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | todoThe `\Redis::xreadgroup()` method waits until a message arrives or the specified timeout is reached before returning, which means that `RedisExt\Connection::get()` is blocking.That's inconsistent with other transports which all returns immediately in case there is no message, for instance the AMQP transport uses `\Amqp::get()` instead of `\Amqp::consume()` for this reason.It also short-circuits the worker's stop logic: both the `--time-limit` option of the `messenger:consume` command and the `messenger:stop-workers` don't work with the redis transport.This returns early in case the message count is 0 and no blocking timeout has been configured.Commits-------229502a [Messenger] Make redis Connection::get() non blocking by default
Uh oh!
There was an error while loading.Please reload this page.
The
\Redis::xreadgroup()method waits until a message arrives or the specified timeout is reached before returning, which means thatRedisExt\Connection::get()is blocking.That's inconsistent with other transports which all returns immediately in case there is no message, for instance the AMQP transport uses
\Amqp::get()instead of\Amqp::consume()for this reason.It also short-circuits the worker's stop logic: both the
--time-limitoption of themessenger:consumecommand and themessenger:stop-workersdon't work with the redis transport.This returns early in case the message count is 0 and no blocking timeout has been configured.