Skip to content

[Messenger] Negative publisher confirm swallowed #53229

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
Norbomat opened this issue Dec 27, 2023 · 3 comments
Closed

[Messenger] Negative publisher confirm swallowed #53229

Norbomat opened this issue Dec 27, 2023 · 3 comments

Comments

@Norbomat
Copy link

Norbomat commented Dec 27, 2023

Symfony version(s) affected

<=7

Description

For reliable message publishing, RabbitMQ offers the possibility to enable publisher confirms on a channel. If enabled, the success of processing a message will be confirmed by the broker. If the broker responds with ack, all is fine. In case of receiving nack however, the client is expected to re-publish the message.

Unfortunately, a nack seems to be swallowed by Messenger:
In this line, Messenger waits for the confirm. The waitForConfirm() method then invokes the callback for ack or nack. Both callbacks have been provided here.

As both callbacks just return "false", no further handling of a nack-case is done. There's no way for code which sends a message to know if this has been successful or not.

The expectation would be that at least an exception is thrown in that case. An even better implementation would perform some retries before eventually throwing an exception.

How to reproduce

When a queue is limited and rejects new messages, the publisher is informed by a nack response. This behavior could easily be provoked by creating an overflow scenario.

messenger.yaml

...
    myTransport:
        dsn: amqp://...
        options:
          confirm_timeout: 3 # enable publisher confirms

Assigning a policy in RabbitMQ:
Either assign a policy like explained here. Or use the admin frontend, click "Admin/Policies/Add policy" and enter the values

  • Name: SomeName
  • Definitions:
    • max-lenght=2
    • overflow=reject-publish

Start a SymfonyMessanger consumer

console messenger:consume myTransport

Sending some messages

class MyMessage
{
    public function __construct(readonly string $text){}
}

#[AsMessageHandler]
class MyMessageHandler
{
    public function __construct(private readonly LoggerInterface $logger)
    {
    }

    public function __invoke(MyMessage $message): void
    {
        $this->logger->info("Message received: " . $message->text);
    }
}

for ($i = 0; $i < 100; $i++) {
    $this->messageBus->dispatch(new MyMessage("Message Nr. $i"));
}

Result
-> Only some message are received. The publisher doesn't get any info that it should retry publishing.

Possible Solution

No response

Additional Context

No response

@lermontex
Copy link

I have a similar issue, but I'm using Symfony 7.0 with RabbitMQ 3.12.12

It looks like everything is working fine, but in fact some messages are getting lost during processing

@kvrushifa
Copy link
Contributor

@lermontex were you able to solve this problem somehow?

@lermontex
Copy link

@kvrushifa, no. Looks like this is a symfony/amqp-messenger bug, as @Norbomat wrote above

nicolas-grekas added a commit that referenced this issue Jan 23, 2024
…ushifa)

This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Messenger] [AMQP] Throw exception on `nack` callback

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #53229
| License       | MIT

If the channel is in confirm mode, it is currently not possible to determine if a message has been nack'ed. By throwing an exception within the confirm callback, it is at least possible to react to it.

Return false is not needed to end the wait loop, [since the amqp ext checks for exceptions](https://github.com/php-amqp/php-amqp/blob/e58ac221e317c840ee06f7731e0dc76c7d6a431a/amqp_methods_handling.c#L249).

Commits
-------

6ed35b6 [Messenger] [AMQP] Throw exception on `nack` callback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants