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

Jump to bottom

[Messenger][Redis] Transport can claim only single message per claim_interval #49023

Open
AdamKatzDev opened this issue Jan 18, 2023 · 4 comments
Open

[Messenger][Redis] Transport can claim only single message per claim_interval #49023

AdamKatzDev opened this issue Jan 18, 2023 · 4 comments
Labels
Bug Messenger Status: Needs Review

Comments

@AdamKatzDev
Copy link

Symfony version(s) affected

5.4, 6.3

Description

https://github.com/symfony/symfony/blob/6.3/src/Symfony/Component/Messenger/Bridge/Redis/Transport/Connection.php

After a message is claimed nextClaim field is reseted:

symfony/src/Symfony/Component/Messenger/Bridge/Redis/Transport/Connection.php

Lines 285 to 303 in 1f7bc10

if (\count($claimableIds) > 0) {
try {
$this->getRedis()->xclaim(
$this->stream,
$this->group,
$this->consumer,
$this->redeliverTimeout,
$claimableIds,
['JUSTID']
);
$this->couldHavePendingMessages = true;
} catch (\RedisException $e) {
throw new TransportException($e->getMessage(), 0, $e);
}
}
$this->nextClaim = microtime(true) + $this->claimInterval;
}

which does not allow to claim next messages:

symfony/src/Symfony/Component/Messenger/Bridge/Redis/Transport/Connection.php

Lines 337 to 339 in 1f7bc10

if (!$this->couldHavePendingMessages && $this->nextClaim <= microtime(true)) {
$this->claimOldPendingMessages();
}

This logic might be intended and works in many cases but it doesn't work well for batch workers with large batch size or large claim_interval setting. It can potentially cause messages to pile up in worst cases or to be processed far too late.

How to reproduce

Possible to reproduce this by SIGKILL'ing a batch worker with messages stored. I hope that my explanation is good enough and a reproducer is not needed.

Possible Solution

Should be possible to fix this by moving nextClaim update to a different place. The field should not be updated if there is a pending message with the same consumer (already does) or a message from another consumer was claimed (this bug).

Additional Context

No response
@xabbuh
Copy link
Member

xabbuh commented Jan 18, 2023

Are you up to open a PR with the solution that you have in mind @AdamKatzDev?

@AdamKatzDev AdamKatzDev mentioned this issue Jan 18, 2023
Open
@AdamKatzDev
Copy link
Author

@xabbuh, done.

@AdamKatzDev
Copy link
Author

Nope, not done, need to consider more things.

@AdamKatzDev
Copy link
Author

The PR should be done now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Messenger Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@xabbuh @carsonbot @AdamKatzDev