Skip to content

[Messenger] Negative delay with Doctrine bridge can't be used #50833

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
Sinjonathan opened this issue Jun 30, 2023 · 1 comment
Closed

[Messenger] Negative delay with Doctrine bridge can't be used #50833

Sinjonathan opened this issue Jun 30, 2023 · 1 comment

Comments

@Sinjonathan
Copy link

Symfony version(s) affected

5.4.25

Description

When we send envelop with negative delay with messenger configured with Redis bridge, messages are immediately processed.

But it's not the case with Doctrine bridge because here, available_at column of the database queue table is wrongly set.

How to reproduce

First we calc a $delayStamp that is representing the delay between the expected execution time (x day in the past) and the current time when we create the envelop (now).

Because of the expected execution time is in the past, we want it to be executed with no delay.

$negativeDelayStamp = (($expectedExecuteAt->getTimestamp() - (new DateTime('now'))->getTimestamp()) * 1000);

Here we dispatch a simple envelop with a negative DelayStamp

$this->messageBus->dispatch(
            new Envelope(new MyMessage(), [
                new DelayStamp($negativeDelayStamp),
            ]));

Next in Symfony\Component\Messenger\Bridge\Doctrine\Transport\Connection $availableAt is calculated using the delay sent :

    public function send(string $body, array $headers, int $delay = 0): string
    {
        $now = new \DateTimeImmutable();
        $availableAt = $now->modify(sprintf('+%d seconds', $delay / 1000));

        $queryBuilder = $this->driverConnection->createQueryBuilder()
            ->insert($this->configuration['table_name'])
            ->values([
                'body' => '?',
                'headers' => '?',
                'queue_name' => '?',
                'created_at' => '?',
                'available_at' => '?',
            ]);
            
            ...

Because of the + in sprintf('+%d seconds', $delay / 1000) , the negative delay will be interpreted positively (ex: -1000 will be interpreted as +1000).

So instead of set availableAt at the expected execution time, x second in the past, it will set availableAt at the x second in the future.

Possible Solution

I see multiple solution here :

  • remove the + in $availableAt = $now->modify(sprintf('+%d seconds', $delay / 1000));

  • throw an exception if negative delay is not a wanted feature

  • do nothing and let the devs verify the delay value before they sent the envelop

Additional Context

No response

@MatTheCat
Copy link
Contributor

MatTheCat commented Jul 1, 2023

Negative delays in DelayStamp are tested since #38484 and I admit I don't see which use-cases this covers 🤔 I guess this would even make some transports crash but none seem to guard against that. Do you recall about this @Nyholm?

Note that you can keep the sign by moving the + after %: https://3v4l.org/uK49F

fabpot added a commit that referenced this issue Dec 24, 2023
This PR was squashed before being merged into the 5.4 branch.

Discussion
----------

[Messenger] Fix using negative delay

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

Fixes using negative delays when sending messages. Cherry-picked from #53088.

Commits
-------

af1517e [Messenger] Fix using negative delay
@fabpot fabpot closed this as completed Dec 24, 2023
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