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

[minor] give a name to the transport in the Worker #34

Merged
merged 1 commit into from
Dec 30, 2021

Conversation

nikophil
Copy link
Member

Hello!

I hope this was not made on purpose, but in order to best mimic the behavior of messenger, the receivers need to have a name in the worker, as seen in Symfony\Component\Messenger\Command\ConsumeMessagesCommand

// Symfony\Component\Messenger\Command\ConsumeMessagesCommand
protected function execute(InputInterface $input, OutputInterface $output): int
{
        $receivers = [];
        foreach ($receiverNames = $input->getArgument('receivers') as $receiverName) {
            //...
            $receivers[$receiverName] = $this->receiverLocator->get($receiverName);
        }

        // ...
        $worker = new Worker($receivers, $bus, $this->eventDispatcher, $this->logger);
        // ...
}

Otherwise, we cannot reproduce the retry behavior because the computation of retryStrategy is based on transport's name. Yet, IMO, it could be a good thing to be able to reproduce retry behavior in tests since I've already had some complex bugs in retry scenario.

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

(Rebase needed)

It was unintentional to leave out the name so this is a valid fix.

Is this related to #2? Should/could we add tests to ensure failed messages are added to a failed transport. I'm not saying this should be done here in this PR, I'm curious for your opinion.

tests/InteractsWithMessengerTest.php Outdated Show resolved Hide resolved
tests/Fixture/config/multi_transport.yaml Show resolved Hide resolved
@nikophil
Copy link
Member Author

I've rebased the PR.

Is this related to #2?

I think this isn't related to #2. Failure transport is only related to retry if the event is setForRetry

Should/could we add tests to ensure failed messages are added to a failed transport.

I think we should not behave as if it does not exist, and then at least configure max_retries: 1 for one transport and test that in blocking and unblocking modes a failing message is dispatched twice

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

Perfect, thanks for the explanation!

@kbond kbond merged commit 13376bb into zenstruck:1.x Dec 30, 2021
kbond added a commit to kbond/messenger-test that referenced this pull request Jan 7, 2022
This fixes a BC break introduced in zenstruck#34. That PR fixed an issue where
the transport names were not being properly registered with the Worker.
This enabled the retry system but broke code that was not expecting
retries to be made.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants