[Messenger] Introduce DeduplicateMiddleware
#54141
Conversation
VincentLanglet
commented
Mar 3, 2024
•
| Q | A |
|---|---|
| Branch? | 7.3 |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | no |
| Issues | Fix #54126 |
| License | MIT |
306a8e7 to
0b67664
Compare
e04298b to
05dcb17
Compare
495364f to
e8b1f4f
Compare
| $envelope = $stack->next()->handle($envelope, $stack); | ||
| } finally { | ||
| // If we've received a lockable message, we're releasing it. | ||
| if (null !== $envelope->last(ReceivedStamp::class)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
For $this->releaseLock($envelope, true); the check on the ReceivedStamp is done because I do:
if (null === $envelope->last(ReceivedStamp::class)) {
// foo
} else {
$this->releaseLock($envelope, true);
}
And releaseLock is not doing the any check on ReceivedStamp, but on LockStamp.
If I don't do the ReceivedStamp check, I will release the lock right after adding the lock (during the first dispatch) instead of when I receive the message.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
5cf5b1d to
d1ae96c
Compare
src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTestCase.php
Outdated
Show resolved
Hide resolved
|
Oh, and a line is missing in the changelog |
b48b5bb to
41c0b2f
Compare
|
@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:
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? |
LockMiddlewareDeduplicateMiddleware
2b1da0d to
b125c9d
Compare
|
Hi @nicolas-grekas @jderusse @welcoMattic @xabbuh It's unclear to me how to move this forward. |
There was a problem hiding this comment.
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!
b125c9d to
3f68223
Compare
3f68223 to
aa7000a
Compare
Rebased and squashed |
|
Thank you @VincentLanglet. |
| 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 { |
There was a problem hiding this comment.
update the test deps next time to avoid these exra conditions :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
i dont like the if/else condition based on class presence generally
There was a problem hiding this comment.
7.2 is tested without class X
7.3 is tested with class X
it's that simple to me.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.