Skip to content

[Messenger] Growing size of ErrorDetailsStamp breaks AMQP for RecoverableMessageHandlingException #45944

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

Open
MantraMediaMike opened this issue Apr 5, 2022 · 10 comments · May be fixed by #53454

Comments

@MantraMediaMike
Copy link

MantraMediaMike commented Apr 5, 2022

Symfony version(s) affected

6.0.3

Description

After the 4th or 5th retry the ErrorDetailsStamp header of a message that was sent back to the queue using the RecoverableMessageHandlingException grows to 60k and RabbitMQ rejects the message with the error

Library error: table too large for buffer

I did find a related and merged issue (#34082) but this does not seem to have solved it, there are recent comments from people who ran into the same issue.

I am able to solve this in using a custom serializer that removes the ErrorDetailsStamp but I think that this is not expected behaviour.

This also has implications for the other transports like Doctrine where very large entries for simple messages are being inserted into the databae.

How to reproduce

Just create a TestMessage and watch the ErrorDetailsStamp header grow out of proportion:

final class TestMessageHandler .....

public function __invoke(TestMessage $message): int
{
  throw new RecoverableMessageHandlingException('Recovered at '.time());
}

Possible Solution

Create a custom serializer and remove the ErrorDetailsStamp in the encode method:

class WorkaroundSerializer ...

public function encode(Envelope $envelope): array
{
.....
  $filteredStamps = [];
  $envelopeStamps = $envelope->all();

  foreach ($envelopeStamps as $stamps) {
    foreach ($stamps as $stamp) {
      if (!$stamp instanceof ErrorDetailsStamp) {
        $filteredStamps[] = $stamp;
      }
     }
   }

  return ... 
}

I guess another solution is to not even add the ErrorDetailsStamp to a user initiated recoverable message as it is not really an error but intended behaviour? Or make headers configurable, eg traceCount: 1 to only add the latest trace or none with 0?

Does it make logical sense that this is an Exception and not a stamp? With it being an Exception although not being an error it also has to be removed from monitoring tools (Sentry etc.) to not be a false negative.

Additional Context

No response

@doubbz
Copy link

doubbz commented Apr 12, 2022

We had the same issue. It's quite troublesome because when it happens the message can be lost forever..

The feature @Mantra-Media is suggesting would be really nice. Meanwhile users should be aware of this issue.

Otherwise, another way of getting around the bug is to create a custom FlattenExceptionNormalizer in which case you won't have to override the whole transport serializer. Keep in mind that you will have to prioritize it before the native one. You will also have to make sure your normalizer only supports when the context includes key messenger_serialization set by the transport serializer. You can also load a configuration file if you want to. At this point it will start to look like @Mantra-Media's suggested feature.

This is still a "get around" solution as you will be coupled with vendor FlattenException class and subject to breaking change.

@enumag
Copy link
Contributor

enumag commented Jun 27, 2022

The workaround I'm using currently is this:

services:
    messenger.failure.add_error_details_stamp_listener:
        class: App\Library\Symfony\Messenger\AddErrorDetailsStampListener
<?php

declare(strict_types=1);

namespace App\Library\Symfony\Messenger;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\Messenger\Event\WorkerMessageFailedEvent;
use Symfony\Component\Messenger\Exception\HandlerFailedException;
use Symfony\Component\Messenger\Stamp\ErrorDetailsStamp;

/**
 * This class is meant to be used instead of \Symfony\Component\Messenger\EventListener\AddErrorDetailsStampListener.
 * The difference is that the ErrorDetailsStamp will be created without FlattenException.
 * This is important because FlattenException contains stack trace which can be quite large,
 * potentially causing AmqpSender to throw "Library error: table too large for buffer".
 */
final class AddErrorDetailsStampListener implements EventSubscriberInterface
{
    public function onMessageFailed(WorkerMessageFailedEvent $event): void
    {
        $throwable = $event->getThrowable();
        if ($throwable instanceof HandlerFailedException) {
            $throwable = $throwable->getPrevious();
        }

        if ($throwable === null) {
            return;
        }

        $stamp = new ErrorDetailsStamp($throwable::class, $throwable->getCode(), $throwable->getMessage());

        $previousStamp = $event->getEnvelope()->last(ErrorDetailsStamp::class);

        // Do not append duplicate information
        if ($previousStamp === null || ! $previousStamp->equals($stamp)) {
            $event->addStamps($stamp);
        }
    }

    public static function getSubscribedEvents(): array
    {
        return [
            // must have higher priority than SendFailedMessageForRetryListener
            WorkerMessageFailedEvent::class => ['onMessageFailed', 200],
        ];
    }
}

@evolic
Copy link

evolic commented Aug 9, 2022

@enumag Great solution! Big thanks!

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@enumag
Copy link
Contributor

enumag commented Mar 2, 2023

Yes there is a workaround as I described above but I'd prefer a better solution.

@carsonbot carsonbot removed the Stalled label Mar 2, 2023
@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@enumag
Copy link
Contributor

enumag commented Sep 4, 2023

The status didn't change since you last asked.

@carsonbot carsonbot removed the Stalled label Sep 4, 2023
BentiGorlich added a commit to MbinOrg/mbin that referenced this issue Jun 20, 2024
- add a new class that adds a lot less exception stamp details to prevent the "Table too large for buffer" error.

original author: https://github.com/enumag
code taken from: symfony/symfony#45944 (comment)
@melroy89
Copy link

I really hope Symfony could fix this issue.

@VimS
Copy link

VimS commented Aug 8, 2024

Did someone investigate this further?

@VimS
Copy link

VimS commented Aug 8, 2024

Found this: #44943 it is the same issue only for different Transports.

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.

7 participants