10000 [Scheduler][Messenger] Message can't be redispatched with Symfony serializer in between · Issue #53562 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Scheduler][Messenger] Message can't be redispatched with Symfony serializer in between #53562

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
christian-kolb opened this issue Jan 17, 2024 · 9 comments · Fixed by #59679
Closed

Comments

@christian-kolb
Copy link
christian-kolb commented Jan 17, 2024

Symfony version(s) affected

6.4.0

Description

When using the RedispatchMessage in combination with the scheduler and the symfony serializer (with JSON), then the messenger fails while trying to deserialize the message. This is due to the MessageContext class which expects the TriggerInterface for the $trigger property. When the serializer serialized the message to JSON, the information which implementation of the trigger is used, is lost.
I think we're missing a custom normalizer for the MessageContext that is able to infer the correct implementation depending on the content.

This is not a problem when the default PHP serialization is used as the relevant class information is backed into the serialized content. I guess that's also the reason that this issue didn't come up yet.

How to reproduce

Create a schedule with any message that transfers to a different transport (so that the message is written to a message queue).

use Symfony\Component\Messenger\Message\RedispatchMessage;
use Symfony\Component\Scheduler\Attribute\AsSchedule;
use Symfony\Component\Scheduler\RecurringMessage;
use Symfony\Component\Scheduler\Schedule;
use Symfony\Component\Scheduler\ScheduleProviderInterface;

#[AsSchedule('default')]
final readonly class ScheduleProvider implements ScheduleProviderInterface
{
    #[\Override]
    public function getSchedule(): Schedule
    {
        $schedule = new Schedule();
        $schedule
            ->add(RecurringMessage::every(
                '5 seconds', // Just for easier testing
                new RedispatchMessage(
                    new DeleteOldNotificationsMessage(), // Class is in same directory
                    'async',
                ),
            ));

        return $schedule;
    }
}

Configure the messenger to use the Symfony serializer like shown in the documentation:

return static function (FrameworkConfig $framework, ContainerConfigurator $container) {
    $messenger = $framework->messenger();

    // -- Serializer
    $messenger
        ->serializer()
        ->defaultSerializer('messenger.transport.symfony_serializer')
        ->symfonySerializer()
            ->format('json');

    // Async - All low priority tasks
    $messenger
        ->transport('async')
        ->dsn('doctrine://default')
        ->options([
            'queue_name' =>'async',
        ]);
    ;
...

Make sure to run the async messenger queue and at the then start the scheduler:

php bin/console messenger:consume -v scheduler_default

Then the job queue will fail with the following error stack:

In Serializer.php line 127:
2024-01-17T12:13:27.950138967Z                                                                                
2024-01-17T12:13:27.950143217Z   [Symfony\Component\Messenger\Exception\MessageDecodingFailedException]       
2024-01-17T12:13:27.950146133Z   Could not decode stamp: The type of the "trigger" attribute for class "Symf  
2024-01-17T12:13:27.950148925Z   ony\Component\Scheduler\Generator\MessageContext" must be one of "Symfony\C  
2024-01-17T12:13:27.950151717Z   omponent\Scheduler\Trigger\TriggerInterface" ("array" given).                
2024-01-17T12:13:27.950154467Z                                                                                
2024-01-17T12:13:27.950157092Z 
2024-01-17T12:13:27.950159425Z Exception trace:
2024-01-17T12:13:27.950161800Z   at /var/www/html/vendor/symfony/messenger/Transport/Serialization/Serializer.php:127
2024-01-17T12:13:27.950164300Z  Symfony\Component\Messenger\Transport\Serialization\Serializer->decodeStamps() at /var/www/html/vendor/symfony/messenger/Transport/Serialization/Serializer.php:72
2024-01-17T12:13:27.950167675Z  Symfony\Component\Messenger\Transport\Serialization\Serializer->decode() at /var/www/html/vendor/symfony/doctrine-messenger/Transport/DoctrineReceiver.php:138
2024-01-17T12:13:27.950170467Z  Symfony\Component\Messenger\Bridge\Doctrine\Transport\DoctrineReceiver->createEnvelopeFromData() at /var/www/html/vendor/symfony/doctrine-messenger/Transport/DoctrineReceiver.php:65
2024-01-17T12:13:27.950173467Z  Symfony\Component\Messenger\Bridge\Doctrine\Transport\DoctrineReceiver->get() at /var/www/html/vendor/symfony/doctrine-messenger/Transport/DoctrineTransport.php:42
2024-01-17T12:13:27.950176300Z  Symfony\Component\Messenger\Bridge\Doctrine\Transport\DoctrineTransport->get() at /var/www/html/vendor/symfony/messenger/Worker.php:102
2024-01-17T12:13:27.950189550Z  Symfony\Component\Messenger\Worker->run() at /var/www/html/vendor/symfony/messenger/Command/ConsumeMessagesCommand.php:238
2024-01-17T12:13:27.950192425Z  Symfony\Component\Messenger\Command\ConsumeMessagesCommand->execute() at /var/www/html/vendor/symfony/console/Command/Command.php:326
2024-01-17T12:13:27.950195175Z  Symfony\Component\Console\Command\Command->run() at /var/www/html/vendor/symfony/console/Application.php:1096
2024-01-17T12:13:27.950197842Z  Symfony\Component\Console\Application->doRunCommand() at /var/www/html/vendor/symfony/framework-bundle/Console/Application.php:126
2024-01-17T12:13:27.950204300Z  Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at /var/www/html/vendor/symfony/console/Application.php:324
2024-01-17T12:13:27.950207008Z  Symfony\Component\Console\Application->doRun() at /var/www/html/vendor/symfony/framework-bundle/Console/Application.php:80
2024-01-17T12:13:27.950209592Z  Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /var/www/html/vendor/symfony/console/Application.php:175
2024-01-17T12:13:27.950212175Z  Symfony\Component\Console\Application->run() at /var/www/html/bin/console:40

Possible Solution

Having a custom normalizer that can denormalize the TriggerInterface depending on the context. That of course would not solve the issue when someone implements their own trigger. But then it's "simple" to construct a custom normalizer for the project.

This is a simplified version of such a normalizer that specifically only handles PeriodicalTrigger (which fixes my local problems):

final class TriggerDenormalizer implements DenormalizerInterface
{
    /**
     * @param array{
     *   intervalInSeconds: int,
     *   from: string,
     *   until: string,
     *   description: string,
     * } $data
     */
    public function denormalize($data, string $type, string $format = null, array $context = []): TriggerInterface
    {
        return new PeriodicalTrigger(
            $data['intervalInSeconds'],
            $data['from'],
            $data['until'],
        );
    }

    public function supportsDenormalization($data, string $type, string $format = null, array $context = []): bool
    {
        return $type === TriggerInterface::class;
    }

    public function getSupportedTypes(?string $format): array
    {
        return [
            TriggerInterface::class => true,
        ];
    }
}

Additional Context

The primary question I'm having is about the approach. Does this makes sense to have an additional normalizer? If so, does it make sense to extend the triggers with logic for normalization and denormalization? They are very flexible to use from the outside, but as a side effect not very "normalizable". The example I've added is a poor man version that doesn't cover a lot of the internals and I'm not sure whether with the current architecture it even could be handled from the outside or whether we need some kind of logic exposed to the outside specific to normalization.
Would ask for direction here.

@mikemix
Copy link
mikemix commented Jan 18, 2024

As a temporary workaround I've included a middleware in the command bus to remove the stamp that's causing the issue:

final readonly class SchedulerStampRemovingMiddleware implements MiddlewareInterface
{
    public function handle(Envelope $envelope, StackInterface $stack): Envelope
    {
        return $stack->next()->handle(
            envelope: $envelope->withoutStampsOfType(ScheduledStamp::class),
            stack: $stack,
        );
    }
}

@Gwemox
Copy link
Contributor
Gwemox commented Jun 5, 2024

Should the trigger property be serialized as a string and not as an object?

  1. TriggerInterface is Stringable.
  2. It doesn't really make sense to serialize a Stateful Trigger
final class MessageContext
{
    public function __construct(
        public readonly string $name,
        public readonly string $id,
        public readonly string $trigger,
        public readonly \DateTimeImmutable $triggeredAt,
        public readonly ?\DateTimeImmutable $nextTriggerAt = null,
    ) {
    }
}

Any idea @tucksaun ?

@christian-kolb
Copy link
Author

@Gwemox The __toString() method seams to only be there for logging and debugging purposes. Not to use for serialization. There is no fromString or something similar that would hint to such a contract. So I don't think that can be used for such a purpose.

As far as I'm aware the "Symfony way" of doing serialization/deserialization is through normalizers, isn't it? 🙂

@Gwemox
Copy link
Contributor
Gwemox commented Jun 5, 2024

@christian-kolb It is not necessary to serialize the entire trigger object because it is not intended to be used once deserialized. In its current form, if you instantiate it again, you will lose its state. 🙂
The trigger property of the MessageContext should primarily be used for logging and debugging purposes.

@christian-kolb
Copy link
Author
christian-kolb commented Jun 6, 2024

Is there to way to serialize the state? When not using JSON serializer and using the default PHP serializer, that state would still exist, wouldn't it? As a user of the component I would never guess that data is being lost when switching the serializer.

I don't know how to do it (not deep enough in it), but at the moment it just looks like the implementation isn't "completed".

@valtzu
Copy link
Contributor
valtzu commented Aug 9, 2024

Schedule triggers may contain closures nowadays so it is pretty much unserializable, even with native serializer.

To me that string conversion sounded perfectly ok, I bet more people are struggling right now with Scheduler not working at all vs. not being able to get the trigger from the stamp.

If you need the trigger object, I think we should rather add getById method to Schedule, so then you'd fetch the schedule using $name from MessageContext and then fetch RecurringMessage using $id from MessageContext, and then call getTrigger() on it.

I'm considering submitting a PR on that since for many people this is really blocking the upgrade to 6.4.

@tucksaun
Copy link
Contributor
tucksaun commented Sep 6, 2024

Should the trigger property be serialized as a string and not as an object?

  1. TriggerInterface is Stringable.
  2. It doesn't really make sense to serialize a Stateful Trigger
final class MessageContext
{
    public function __construct(
        public readonly string $name,
        public readonly string $id,
        public readonly string $trigger,
        public readonly \DateTimeImmutable $triggeredAt,
        public readonly ?\DateTimeImmutable $nextTriggerAt = null,
    ) {
    }
}

Any idea @tucksaun ?

IIRC having the trigger available was an idea from @kbond and this looked sensible to me back then.
But this does not seem to be used within Symfony itself so...
But we can't remove the tigger at this would be a BC-break, I think, I'm not sure of the extend of this assertion as the class is final and maybe not really instantiated outside of Symfony but not sure.
I think (but to be confirmed) that we might allow it to become nullable without BC break because the class is final.

Without a change on the context, indeed this means that when using RedispatchMessage one needs a serializable trigger (or use native PHP serialization?)
The problem I see is that we can not force TriggerInterface to be serializable either because that would be another BC break (and then we would to ensure they all are serializable).

While we look for a good Symfony solution (or wait to be allowed for a BC), the safest way here could be to have a small handler for the recurring message that is handled synchronously and does the redispatch ?

@kbond
Copy link
Member
kbond commented Sep 16, 2024

Since the trigger isn't required anymore at this point, could we create a normalizer that normalizes to the string, then denormalizes to a stub object - maybe just an anon. class that implements the interface, throws an exception in getNextRunDate() and returns the string in __toString()?

To account for the possibility of Closures in the trigger causing serialization problems, perhaps something similar to above in MessageContext::__serialize()/__unserialize()?

@kbond kbond changed the title [Scheduler|Messenger] Message can't be redispatched with Symfony serializer in between [Scheduler][Messenger] Message can't be redispatched with Symfony serializer in between Sep 16, 2024
@valtzu
Copy link
Contributor
valtzu commented Feb 3, 2025

I have implemented @kbond's suggestion in #59679

@fabpot fabpot closed this as completed in 277486a Feb 7, 2025
symfony-splitter pushed a commit to symfony/scheduler that referenced this issue Feb 7, 2025
…valtzu)

This PR was merged into the 7.3 branch.

Discussion
----------

[Scheduler] Normalize `TriggerInterface` as `string`

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

Fix the issue with Scheduler & Serializer like `@kbond` described in symfony/symfony#53562 (comment).

Fixes the following error:

```
Could not decode stamp: The type of the "trigger" attribute for class "Symfony\Component\Scheduler\Generator\MessageContext" must be one of "Symfony\Component\Scheduler\Trigger\TriggerInterface" ("array" given).
```

Commits
-------

3aa47e0ada2 Normalize `TriggerInterface` as `string`
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.

8 participants
0