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

[Messenger][AMQP] Use delivery_mode=2 by default #34956

Merged
merged 1 commit into from Dec 15, 2019

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Dec 12, 2019

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #34685
License MIT
Doc PR

@lyrixx
Copy link
Member Author

lyrixx commented Dec 12, 2019

I have tested it:

<?php

use Symfony\Component\Messenger\Transport\AmqpExt\AmqpStamp;
use Symfony\Component\Messenger\Transport\AmqpExt\Connection;

require __DIR__.'/vendor/autoload.php';

$connection = Connection::fromDsn('amqp://rabbitmq-3.lxd?exchange[name]=test');
$connection->publish('body', [], 0, new AmqpStamp('test'));

I have created an exchange (fanout) and a queue by hand.


This is not related, but I notice something strange: The component created the a queue messages for me :/ Why? Where does "messages" comes? I know it's the default exchange name but in my case, I forced the exchange name to test and my routing key was test. More over the exchange was in a fanout mode, so IMHO it does really make sens here to create the queue since a queue is already bound. Is there a way to avoid that?

OK, I tried

$connection = Connection::fromDsn('amqp://rabbitmq-3.lxd?exchange[name]=test&queues[test]=&queues[test2]=');
$connection->publish('body', [], 0, new AmqpStamp('test'));

And it does what I want 👍

So now I have another question: If I want to publish to 2 different exchanges, I have to create 2 connection?

@pyrech
Copy link
Contributor

pyrech commented Dec 13, 2019

Shouldn't this PR be considered as a bug fix? If not, that means that Symfony 4.4 & 5.0 will provide an AMQP connection that is not safe to use by default.

@lyrixx
Copy link
Member Author

lyrixx commented Dec 13, 2019

I agree with @pyrech here.

To be really clear : Right now if you restart rabbitmq, you will loose all messages. This is really not safe.

@fabpot Are you OK to treat this PR as a bug fix ?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

As bug fix on 4.4/4.3 as it fits.

@lyrixx lyrixx changed the base branch from master to 4.3 December 13, 2019 13:43
@lyrixx
Copy link
Member Author

lyrixx commented Dec 13, 2019

I updated the PR to target 4.3

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Dec 13, 2019
@fabpot
Copy link
Member

fabpot commented Dec 15, 2019

Thank you @lyrixx.

fabpot added a commit that referenced this pull request Dec 15, 2019
This PR was merged into the 4.3 branch.

Discussion
----------

[Messenger][AMQP] Use delivery_mode=2 by default

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34685
| License       | MIT
| Doc PR        |

Commits
-------

be2eb6f [Messenger][AMQP] Use delivery_mode=2 by default
@fabpot fabpot merged commit be2eb6f into symfony:4.3 Dec 15, 2019
@lyrixx lyrixx deleted the messenger-delivery branch December 15, 2019 21:17
@weaverryan
Copy link
Member

About the “messages” queue being created, you could create a separate issue for that. There is a queues config option and I think you could set it to an empty array to avoid this (but I could be wrong). In general, there are many different ways for using Amqp - and so we need to make it work as smoothly as possible for as many cases as we can.

This was referenced Dec 19, 2019
@Tobion
Copy link
Member

Tobion commented Jan 20, 2020

Seems fine to do this by default as many people don't know the default settings of AMQP it seems. But it will make the AMQP way slower. I used a transport decorator for this so far when I needed it.

/**
 * Decorates a transport and makes all messages getting sent persistent.
 */
class PersistentMessagesTransportDecorator implements TransportInterface, SetupableTransportInterface
{
    private const DELIVERY_MODE_PERSISTENT = 2;

    /**
     * @var TransportInterface
     */
    private $decoratedTransport;

    public function __construct(TransportInterface $decoratedTransport)
    {
        $this->decoratedTransport = $decoratedTransport;
    }

    public function setup(): void
    {
        if ($this->decoratedTransport instanceof SetupableTransportInterface) {
            $this->decoratedTransport->setup();
        }
    }

    public function get(): iterable
    {
        return $this->decoratedTransport->get();
    }

    public function ack(Envelope $envelope): void
    {
        $this->decoratedTransport->ack($envelope);
    }

    public function reject(Envelope $envelope): void
    {
        $this->decoratedTransport->reject($envelope);
    }

    public function send(Envelope $envelope): Envelope
    {
        return $this->decoratedTransport->send($this->addPersistentDeliveryModeToEnvelope($envelope));
    }

    private function addPersistentDeliveryModeToEnvelope(Envelope $envelope): Envelope
    {
        /** @var AmqpStamp|null $amqpStamp */
        $amqpStamp = $envelope->last(AmqpStamp::class);
        $attributes = $amqpStamp ? $amqpStamp->getAttributes() : [];
        $attributes['delivery_mode'] = self::DELIVERY_MODE_PERSISTENT;

        $amqpStampWithDeliveryMode = new AmqpStamp(
            $amqpStamp ? $amqpStamp->getRoutingKey() : null,
            $amqpStamp ? $amqpStamp->getFlags() : \AMQP_NOPARAM,
            $attributes
        );

        return $envelope->with($amqpStampWithDeliveryMode);
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants