Skip to content

Scheduler doesn't store state #50669

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.

Sign up for GitHub

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

Jump to bottom
Closed
kamil-karkus opened this issue Jun 15, 2023 · 5 comments
Closed

Scheduler doesn't store state #50669

kamil-karkus opened this issue Jun 15, 2023 · 5 comments
Labels
Bug Scheduler Status: Needs Review

Comments

@kamil-karkus
Copy link

kamil-karkus commented Jun 15, 2023 •
edited
Loading

Symfony version(s) affected

6.3.0

Description

I don't know if it's a bug or feature.
However if I'll put --time-limit in consumer command or I restart consumer during deployment it re-handles messages that shouldn't be re-handled (defined time has not yet passed)

How to reproduce

<?php

namespace App\Scheduler;

class TestMessage
{

}

<?php

namespace App\Scheduler;

use Psr\Log\LoggerInterface;
use Symfony\Component\Messenger\Attribute\AsMessageHandler;

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

    public function __invoke(TestMessage $message)
    {
        $this->logger->error('Handling message', ['time' => time()]);
    }
}

<?php

namespace App\Scheduler;

use Symfony\Component\Lock\LockFactory;
use Symfony\Component\Scheduler\Attribute\AsSchedule;
use Symfony\Component\Scheduler\RecurringMessage;
use Symfony\Component\Scheduler\Schedule;
use Symfony\Component\Scheduler\ScheduleProviderInterface;
use Symfony\Contracts\Cache\CacheInterface;

#[AsSchedule('test')]
class ScheduleProvider implements ScheduleProviderInterface
{
    public function __construct(
        private LockFactory $lockFactory,
        private CacheInterface $cache,
    ) {

    }

    public function getSchedule(): Schedule
    {
        $schedule = new Schedule();
        $schedule->lock($this->lockFactory->createLock('scheduler'));
        $schedule->stateful($this->cache);
        return $schedule->add(
            RecurringMessage::every('30 seconds', new TestMessage()),
        );
    }
}

NOTE: in my local env lock and cache utilize redis

  1. Run bin/console messenger:consume scheduler_test --time-limit=10 -v
  2. Run it again

Output 1st run

14:01:59 ERROR     [app] Handling message ["time" => 1686830519]

Output 2nd run

14:02:11 ERROR     [app] Handling message ["time" => 1686830531]

Only 12 seconds of difference, and message is registered as "every 30 seconds"

Possible Solution

No response

Additional Context

obraz
@stof
Copy link
Member

stof commented Jun 15, 2023

what is the configuration of your cache ? Do you have a persistent cache like Redis ? Or is it using an ArrayCache that only caches in memory (and so will start the new process with an empty cache) ?

@kamil-karkus
Copy link
Author

As I mentioned in the issue, it's redis

$ bin/console debug:config framework cache

Current configuration for "framework.cache"
===========================================

app: cache.adapter.redis
default_redis_provider: '%env(REDIS_DSN)%'
pools:
    doctrine.result_cache_pool:
        adapters:
            - cache.app
        tags: null
        public: false
    doctrine.system_cache_pool:
        adapters:
            - cache.system
        tags: null
        public: false
    doctrine.second_level_cache_pool:
        adapters:
            - cache.app
        tags: null
        public: false
prefix_seed: _/var/www/application.App_KernelDevDebugContainer
system: cache.adapter.system
directory: /var/www/application/var/cache/dev/pools/app
default_memcached_provider: 'memcached://localhost'
default_doctrine_dbal_provider: database_connection
default_pdo_provider: null

@kamil-karkus
Copy link
Author

obraz

@StanJansen
Copy link
Contributor

I'm having the same issue. Are there any solutions/workarounds for this?

@StanJansen
Copy link
Contributor

Looked at the code; if you do not pass the from argument, it sets it at the current timestamp. If that one is bigger than the last run, it uses that instead. So when you pass a from in the past to the RecurringMessage::every method, it will work with caching.

Not sure if this is intended or not, but it is a (temporary) workaround...

@StanJansen StanJansen mentioned this issue Jun 29, 2023
Merged
nicolas-grekas added a commit that referenced this issue Jul 5, 2023
@nicolas-grekas
bug #50818 [Scheduler] Fix PeriodicalTrigger from argument for stat…
dd83b18 
…eful run dates (StanJansen)

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

Discussion
----------

[Scheduler] Fix `PeriodicalTrigger` from argument for stateful run dates

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #50669
| License       | MIT
| Doc PR        | -

Summary: Unless you provide the `from` argument (with a date in the past), caching will not work for the `PeriodicalTrigger`.

If the `from` argument is not passed to the `RecurringMessage::every` method, it will, fallback to `new \DateTimeImmutable()`. If you use a stateful schedule and you restart the consumer (either manually or using a time limit), the cached last-executed date will always be overridden by the moment you restart the scheduler due to `if ($this->from > $run) {`.

Commits
-------

cb9be9f [Scheduler] Fix `PeriodicalTrigger` from argument for stateful run dates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Scheduler Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@nicolas-grekas @stof @xabbuh @kamil-karkus @StanJansen @carsonbot