Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Messenger] return empty envelopes when RetryableException occurs#32979

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

Conversation

@surikman
Copy link
Contributor

@surikmansurikman commentedAug 6, 2019•
edited
Loading

QA
Branch?3.4 or 4.3 for bug fixes
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
LicenseMIT
Doc PRsymfony/symfony-docs#12109

Problem occurs when you are using more than 1 worker with Doctrine Transport.

Symfony\Component\Messenger\Transport\Doctrine\Connection::get does a querySELECT ... 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.

@surikmansurikmanforce-pushed themessenger-doctrine-use-retryable-exception branch fromb94e988 toa5693f5CompareAugust 6, 2019 08:29
@nicolas-grekasnicolas-grekas added this to the4.3 milestoneAug 6, 2019
@surikmansurikman requested a review fromsroze as acode ownerAugust 6, 2019 16:54
@surikmansurikmanforce-pushed themessenger-doctrine-use-retryable-exception branch from8189b65 to864b51dCompareAugust 6, 2019 16:56
Copy link
Contributor

@TobionTobion left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We should add a safety counter tocatch (RetryableException $exception) so when it's happens like 3 times in a row, we rethrow. Otherwise a permanent deadlock or misconfiguration of timeouts might be invisible as you would always just hide the exception.

surikman reacted with thumbs up emoji
@surikman
Copy link
ContributorAuthor

@Tobion Thanks. I implemented safety counter as you described.

@surikmansurikmanforce-pushed themessenger-doctrine-use-retryable-exception branch fromca322e5 to20cddc3CompareAugust 7, 2019 07:49
@surikmansurikmanforce-pushed themessenger-doctrine-use-retryable-exception branch from877c401 to8e69051CompareAugust 12, 2019 06:46
@surikmansurikmanforce-pushed themessenger-doctrine-use-retryable-exception branch from8e69051 toc64f5aeCompareAugust 17, 2019 08:37
thrownewTransportException($exception->getMessage(),0,$exception);
}

return [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 withwhatever might decorate this receiver... it would "see" an empty array while actually, it was an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@sroze we need your approval here

@Tobion
Copy link
Contributor

Thank you@surikman.

@TobionTobionforce-pushed themessenger-doctrine-use-retryable-exception branch fromc64f5ae to9add32aCompareSeptember 27, 2019 11:21
Tobion added a commit that referenced this pull requestSep 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     <!-- seehttps://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 peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://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
@TobionTobion merged commit9add32a intosymfony:4.3Sep 27, 2019
@surikmansurikman deleted the messenger-doctrine-use-retryable-exception branchSeptember 30, 2019 06:36
@fabpotfabpot mentioned this pull requestOct 7, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot approved these changes

+2 more reviewers

@TobionTobionTobion approved these changes

@srozesrozesroze left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

6 participants

@surikman@Tobion@fabpot@nicolas-grekas@sroze@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp