Skip to content

[mailer] When using the FailoverTransport it not always picks the first defined transport #37593

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

Closed
evertharmeling opened this issue Jul 16, 2020 · 2 comments

Comments

@evertharmeling
Copy link
Contributor

evertharmeling commented Jul 16, 2020

Symfony version(s) affected: 5.1

Description
According to the docs, specifically the following part:

The mailer will start using the first transport. If the sending fails, the mailer won’t retry it with the other transports, but it will switch to the next transport automatically for the following deliveries.

If I understand it correctly, it would always pick the first defined transport and only when this transport fails would use the next defined transport in line.

However this is not the case. This is introduced with the following change 6ebe83c.

How to reproduce
Run the following (a bit of hacky) test multiple times to trigger the 'randomized' code.

    public function testPickingFirstTransport()
    {
        $transport1 = Transport::fromDsn('smtp://mailserver-01:1025');
        $transport2 = Transport::fromDsn('smtp://mailserver-02:1026');

        $failoverTransport = new FailoverTransport([
            $transport1,
            $transport2,
        ]);

        $reflector = new ReflectionClass($failoverTransport);
        $method = $reflector->getMethod('getNextTransport');
        $method->setAccessible(true);

        $usedTransport = $method->invoke($failoverTransport);
        Assert::assertEquals($transport1, $usedTransport);
    }

https://github.com/evertharmeling/failover-transport

Possible Solution
Because the FailoverTransport extends the RoundRobinTransport and triggers the parent::getNextTransport();

  • Do not rely on the parent::getNextTransport() and rely on an own implementation, of picking the transport in defined order and moving to the next when marked as a 'dead transport'.
  • Make the $cursor variable of the RoundRobinTransport protected, so it can be initiated as protected $cursor = 0; in the FailoverTransport. So it would bypass the 'randomized' code defined in RoundRobinTransport::getNextTransport().

Additional context
If I misinterpreted the docs and misjudged the functionality, ignore this issue ;)

@fabpot
Copy link
Member

fabpot commented Jul 20, 2020

AFAIU, the problem is only on 5.1+ (4.4 and 5.0 are not affected).

@fabpot
Copy link
Member

fabpot commented Jul 20, 2020

See #37611 for a fix.

@fabpot fabpot closed this as completed Jul 20, 2020
fabpot added a commit that referenced this issue Jul 20, 2020
This PR was merged into the 5.1 branch.

Discussion
----------

[Mailer] Fix failover transport

| Q             | A
| ------------- | ---
| Branch?       | 5.1 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #37593 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | n/a

This fixes a regression in 5.1.

Commits
-------

8b673f5 [Mailer] Fix failover transport
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants