[Messenger] return empty envelopes when RetryableException occurs #32979
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
b94e988 to
a5693f5
Compare
8189b65 to
864b51d
Compare
Tobion
reviewed
Aug 6, 2019
src/Symfony/Component/Messenger/Transport/Doctrine/DoctrineReceiver.php
Outdated
Show resolved
Hide resolved
Tobion
reviewed
Aug 6, 2019
src/Symfony/Component/Messenger/Transport/Doctrine/DoctrineReceiver.php
Outdated
Show resolved
Hide resolved
Tobion
suggested changes
Aug 6, 2019
surikman
added a commit
to surikman/symfony-docs
that referenced
this pull request
Aug 7, 2019
surikman
added a commit
to surikman/symfony-docs
that referenced
this pull request
Aug 7, 2019
|
@Tobion Thanks. I implemented safety counter as you described. |
ca322e5 to
20cddc3
Compare
Tobion
reviewed
Aug 7, 2019
src/Symfony/Component/Messenger/Transport/Doctrine/Connection.php
Outdated
Show resolved
Hide resolved
Tobion
reviewed
Aug 11, 2019
877c401 to
8e69051
Compare
Tobion
reviewed
Aug 12, 2019
src/Symfony/Component/Messenger/Tests/Transport/Doctrine/DoctrineReceiverTest.php
Show resolved
Hide resolved
8e69051 to
c64f5ae
Compare
Tobion
approved these changes
Aug 17, 2019
sroze
reviewed
Aug 18, 2019
| throw new TransportException($exception->getMessage(), 0, $exception); | ||
| } | ||
|
|
||
| return []; |
There was a problem hiding this comment.
Wouldn't it be better to retry "internally" (i.e. return $this->get()) instead of returning this empty array? Otherwise, in the event of using multiple transports, the worker might start consuming lower priority messages, which would make no sense. Similar with whatever might decorate this receiver... it would "see" an empty array while actually, it was an error.
There was a problem hiding this comment.
Does not seem better to me. This way we can use the delay that is added after the get which is what we want with a deadlock/timeout problem.
fabpot
approved these changes
Sep 27, 2019
|
Thank you @surikman. |
c64f5ae to
9add32a
Compare
Tobion
added a commit
that referenced
this pull request
Sep 27, 2019
… occurs (surikman) This PR was squashed before being merged into the 4.3 branch (closes #32979). Discussion ---------- [Messenger] return empty envelopes when RetryableException occurs | Q | A | ------------- | --- | Branch? | 3.4 or 4.3 for bug fixes <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | License | MIT | ~~Doc PR~~ | ~~symfony/symfony-docs#12109~~ <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against branch 4.4. - Legacy code removals go to the master branch. --> Problem occurs when you are using more than 1 worker with Doctrine Transport. `Symfony\Component\Messenger\Transport\Doctrine\Connection::get` does a query `SELECT ... FOR UPDATE` and this locking query could lock table and workers stops. But using locks can result in dead locks or lock timeouts. Doctrine renders these SQL errors as RetryableExceptions. These exceptions are often normal if you are in a high concurrency environment. They can happen very often and your application should handle them properly. Commits ------- 9add32a [Messenger] return empty envelopes when RetryableException occurs
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Doc PRsymfony/symfony-docs#12109Problem occurs when you are using more than 1 worker with Doctrine Transport.
Symfony\Component\Messenger\Transport\Doctrine\Connection::getdoes a querySELECT ... FOR UPDATEand this locking query could lock table and workers stops. But using locks can result in dead locks or lock timeouts. Doctrine renders these SQL errors as RetryableExceptions. These exceptions are often normal if you are in a high concurrency environment. They can happen very often and your application should handle them properly.