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] IntroduceDeduplicateMiddleware#54141

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

Merged
fabpot merged 1 commit intosymfony:7.3fromVincentLanglet:lockMiddleware
Feb 7, 2025

Conversation

VincentLanglet
Copy link
Contributor

@VincentLangletVincentLanglet commentedMar 3, 2024•
edited by fabpot
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
IssuesFix#54126
LicenseMIT

David-Moisan, kaznovac, jderusse, valtzu, echantigny, pounard, maxhelias, wmouwen, maidmaid, and OskarStark reacted with thumbs up emojiprilka reacted with heart emojimaxhelias reacted with eyes emoji
Copy link
Member

@jderussejderusse left a comment

Choose a reason for hiding this comment

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

I like the idea, and using the lock component provides a simple solution.

I wonder if this should be namedLockMiddleware orDedupplicationMiddleware. At first I though this PR was about avoid processing similar messages in parallel.

pounard reacted with thumbs up emoji
@VincentLangletVincentLangletforce-pushed thelockMiddleware branch 3 times, most recently from495364f toe8b1f4fCompareMarch 14, 2024 00:16
$envelope = $stack->next()->handle($envelope, $stack);
} finally {
// If we've received a lockable message, we're releasing it.
if (null !== $envelope->last(ReceivedStamp::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not necessary, the check is performed in the releaseLock method.
Also, it's not consistent with the call$this->releaseLock($envelope, true); at line 64

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

For$this->releaseLock($envelope, true); the check on theReceivedStamp is done because I do:

if (null === $envelope->last(ReceivedStamp::class)) {    // foo} else {    $this->releaseLock($envelope, true);}

AndreleaseLock is not doing the any check onReceivedStamp, but onLockStamp.
If I don't do theReceivedStamp check, I will release the lock right after adding the lock (during the first dispatch) instead of when I receive the message.

@VincentLanglet
Copy link
ContributorAuthor

Friendly ping@jderusse@ro0NL do you have time for new look ?

@nicolas-grekas
Copy link
Member

Oh, and a line is missing in the changelog

@VincentLangletVincentLangletforce-pushed thelockMiddleware branch 5 times, most recently fromb48b5bb to41c0b2fCompareOctober 25, 2024 07:14
@pounard
Copy link
Contributor

pounard commentedOct 25, 2024•
edited
Loading

@VincentLanglet I want to apologize for having misunderstood the original issue. Please ignore all my comments.

May I suggest renaming the whole to something much more specific and explicit about what it does and why, such as@jderusse suggested in one the first comments?

Some suggestions about how I would name those classes:

  • LockMiddleware ->DeduplicateMiddleware
  • LockStamp ->DeduplicateStamp

Because the "Lock" term may mislead to "contention" or "conflict" problem resolution, such as deadlocks or transaction failures in database for example, but in my opinion doesn't ring any bell about deduplication.

Once the deduplication thing rang a bell in my mind, it completely wiped out the idea of using it for delaying messages or avoiding parallel processing: it is indeed a very different use case, but one that would also be resolved using a lock/mutex. Hence the wrong naming that could mean very different things.

Any opinion about this@nicolas-grekas?

@VincentLangletVincentLanglet changed the title[Messenger] IntroduceLockMiddleware[Messenger] IntroduceDeduplicateMiddlewareOct 25, 2024
@VincentLangletVincentLangletforce-pushed thelockMiddleware branch 3 times, most recently from2b1da0d tob125c9dCompareOctober 25, 2024 10:52
@fabpotfabpot modified the milestones:7.2,7.3Nov 20, 2024
@VincentLanglet
Copy link
ContributorAuthor

Hi@nicolas-grekas@jderusse@welcoMattic@xabbuh

It's unclear to me how to move this forward.
The PR seems ready to me, without existing request change, so I assume it just require some. of your review ?

Copy link
Member

@welcoMatticwelcoMattic 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.

It look good to me. If possible to rebase the branch and squash commits@VincentLanglet, it will make the merge easier. Thanks for your contribution!

@VincentLanglet
Copy link
ContributorAuthor

It look good to me. If possible to rebase the branch and squash commits@VincentLanglet, it will make the merge easier. Thanks for your contribution!

Rebased and squashed

@fabpot
Copy link
Member

Thank you@VincentLanglet.

@fabpotfabpot merged commit0b9c488 intosymfony:7.3Feb 7, 2025
7 of 11 checks passed
Comment on lines +1066 to +1076
if (class_exists(DeduplicateMiddleware::class)) {
$this->assertEquals([
['id' => 'add_bus_name_stamp_middleware', 'arguments' => ['messenger.bus.commands']],
['id' => 'reject_redelivered_message_middleware'],
['id' => 'dispatch_after_current_bus'],
['id' => 'failed_message_processing_middleware'],
['id' => 'deduplicate_middleware'],
['id' => 'send_message', 'arguments' => [true]],
['id' => 'handle_message', 'arguments' => [false]],
], $container->getParameter('messenger.bus.commands.middleware'));
} else {
Copy link
Contributor

@ro0NLro0NLMar 12, 2025•
edited
Loading

Choose a reason for hiding this comment

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

update the test deps next time to avoid these exra conditions :)

Copy link
Contributor

Choose a reason for hiding this comment

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

it's all maintenance

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

There’s not really a need to forbid using FrameworkBundle with Messenger < 7.3. But that would be the consequence if we cannot test it.

Copy link
Contributor

Choose a reason for hiding this comment

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

i dont like the if/else condition based on class presence generally

Copy link
Contributor

@ro0NLro0NLMar 12, 2025•
edited
Loading

Choose a reason for hiding this comment

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

7.2 is tested without class X
7.3 is tested with class X

it's that simple to me.

Copy link
Member

Choose a reason for hiding this comment

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

That would complicate things. Either we would require people to update FrameworkBundle and components at the same time which would make updating harder. Or we would not be able to test the bundle with the lowest deps which would not allow us to detect compatibility issues.

Copy link
Contributor

@ro0NLro0NLMar 12, 2025•
edited
Loading

Choose a reason for hiding this comment

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

we would require people to update FrameworkBundle and components at the same time which would make updating harder

generally yes, it that prevents if/else flows in your codebase. We should prefer branches then.

updating many sf packages from 7.2 to 7.x is not an issue.

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

@pounardpounardpounard left review comments

@yariksheptykinyariksheptykinyariksheptykin left review comments

@ro0NLro0NLro0NL approved these changes

@chalasrchalasrchalasr left review comments

@lermontexlermontexlermontex left review comments

@fabpotfabpotfabpot approved these changes

@welcoMatticwelcoMatticwelcoMattic approved these changes

@jderussejderusseAwaiting requested review from jderusse

@xabbuhxabbuhAwaiting requested review from xabbuh

Assignees
No one assigned
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

[Messenger] Provide a deduplication strategy to Queue
14 participants
@VincentLanglet@echantigny@maidmaid@nicolas-grekas@pounard@fabpot@jderusse@yariksheptykin@welcoMattic@ro0NL@xabbuh@chalasr@lermontex@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp