NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[Scheduler] Fix stateful scheduler#51651

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:6.4fromvaltzu:stateful-scheduler-bugfix

Sep 16, 2023

Merged

[Scheduler] Fix stateful scheduler#51651

fabpot merged 1 commit intosymfony:6.4fromvaltzu:stateful-scheduler-bugfix

Sep 16, 2023

Conversation

Copy link

Contributor

valtzu commentedSep 13, 2023•
edited
Loading

Q	A
Branch?	6.3
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix#51646,#51384
License	MIT

Stateful scheduler seems rather broken at the moment, see#51384 (comment).

Let's fix it by storing the original first run start time, that way it's always possible to recalculate the state.

Catching-up works now:

[23:14:11.709710] Worker started[23:14:11.759318] every 2 seconds[23:14:11.760291] every 2 seconds[23:14:11.761257] every 2 seconds[23:14:11.763244] every 2 seconds[23:14:12.637054] every 2 seconds[23:14:14.620595] every 2 seconds[23:14:16.632170] every 2 seconds

Whereas before it would only start on from the current item, possibly skipping previous items like stated in#51646.(is this a bc break?)

I will be waiting for input from authors of the related issues.

One test is failing because becausegetNextRunDate is called with2020-02-20T01:59:00+02:00 and then next run date is expected at2020-02-20T02:09:00+02:00 but we get2020-02-20T02:00:00+02:00 because that's set asfrom. I don't quite get the logic, I would assume that it is expected to be run immediately onfrom 🤔

valtzu requested a review fromkbond as acode owner

September 13, 2023 20:08

carsonbot added Status: Needs Review Bug Scheduler labels

Sep 13, 2023

carsonbot added this to the6.3 milestone

Sep 13, 2023

valtzu mentioned this pull request

Sep 13, 2023

[Scheduler] Intermittent Runs#51646

Closed

kbond mentioned this pull request

Sep 14, 2023

[Scheduler] Caching does not work for multiple messages in same schedule#51384

Closed

fabpot requested changes

Sep 15, 2023

View reviewed changes

Copy link

Member

fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I was working on the same.
The failing test must be updated as the current one is broken and does not make sense as you already noticed.

src/Symfony/Component/Scheduler/Trigger/PeriodicalTrigger.php Outdated

		$delta = $run->format('U.u') - $from;
		$recurrencesPassed = floor($delta / $this->intervalInSeconds);
		$nextRunTimestamp = sprintf('%.6F', ($recurrencesPassed + 1) * $this->intervalInSeconds + $from);
		$nextRun = \DateTimeImmutable::createFromFormat('U.u', $nextRunTimestamp,$fromDate->getTimezone());
		$nextRun = \DateTimeImmutable::createFromFormat('U.u', $nextRunTimestamp)->setTimezone($fromDate->getTimezone());

Copy link

Member

fabpotSep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It looks like this change is not needed, is it?

Copy link

ContributorAuthor

valtzuSep 15, 2023•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

From my testing, UTC is always forced in the DateTime instance when it is created from unix timestamp (timezone constructor arg has no effect), so to have the same timezone, it must be set after the instance is created.

But you are right in that this fix is not related to the issue and can/should probably be excluded here.

Copy link

Member

fabpotSep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Can you submit another PR on 6.3 for this change then?

src/Symfony/Component/Scheduler/Generator/CheckpointInterface.phpShow resolvedHide resolved

src/Symfony/Component/Scheduler/Trigger/PeriodicalTrigger.php Outdated

		@@ -133,6 +140,6 @@ private function canBeConvertedToSeconds(\DateInterval $interval): bool

		private function calcInterval(\DateInterval $interval): float
		{
		return$this->from->setTimestamp(0)->add($interval)->format('U.u');
		return(new \DateTimeImmutable('@0'))->add($interval)->format('U.u');

Copy link

Member

fabpotSep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Why do we need this change?
We might add(float) to make Psalm happy :)

Copy link

ContributorAuthor

valtzuSep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I originally made this because $this->from was not always available at this point, I'm not sure if that's the case anymore

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Sep 15, 2023

Copy link

Member

fabpot commentedSep 15, 2023

This must be merged on 6.4 (because of the BC break, which is allowed in 6.4 for experimental components, but not for a patch release).

Copy link

ContributorAuthor

valtzu commentedSep 15, 2023•
edited
Loading

I was working on the same.

@fabpot Would you like to take over this, or if you had done work on this already, discard this MR as you probably have better knowledge in this 😅

fabpot modified the milestones:6.3,6.4

Sep 15, 2023

Copy link

Member

fabpot commentedSep 15, 2023

I was working on the same.
@fabpot Would you like to take over this, or if you had done work on this already, discard this MR as you probably have better knowledge in this 😅

Actually, I like your code better, so if you take into account my few comments, I will be able to merge it fast.

Fix stateful scheduler

2d5856b

valtzu force-pushed thestateful-scheduler-bugfix branch fromf761fc9 to2d5856bCompare

September 15, 2023 13:56

valtzu requested review fromxabbuh,lyrixx,dunglas,yceruto,chalasr,OskarStark andjderusse ascode owners

September 15, 2023 13:56

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Sep 15, 2023

valtzu changed the base branch from6.3 to6.4

September 15, 2023 13:57

Copy link

ContributorAuthor

valtzu commentedSep 15, 2023•
edited
Loading

Sorry I think I misunderstood your comment@fabpot and retargeted this to 6.4 😅

The triggers have changed quite much between the 6.3&6.4 branches, but I should still target 6.3 and leave conflict resolution to someone else?

Oh my, I think I have notified everyone... sorry guys!

I will finish for today not to cause any more trouble.

fabpot approved these changes

Sep 15, 2023

View reviewed changes

carsonbot removed the Status: Needs Review label

Sep 15, 2023

carsonbot added the Status: Reviewed label

Sep 15, 2023

fabpot approved these changes

Sep 16, 2023

View reviewed changes

Copy link

Member

fabpot commentedSep 16, 2023

Thank you@valtzu.

fabpot mentioned this pull request

Sep 16, 2023

[Scheduler] Fix stateful schedulersymfony/symfony-docs#18871

Closed

fabpot merged commita90eca6 intosymfony:6.4

Sep 16, 2023

valtzu mentioned this pull request

Sep 16, 2023

[Scheduler] Match next run timezone with "from" timezone#51674

Merged

fabpot added a commit that referenced this pull request

Sep 16, 2023

bug#51674[Scheduler] Match next run timezone with "from" timezone (…

024ba81

…valtzu)This PR was merged into the 6.3 branch.Discussion----------[Scheduler] Match next run timezone with "from" timezone| Q             | A| ------------- | ---| Branch?       | 6.3| Bug fix?      | yes| New feature?  | no| Deprecations? | no| License       | MITAs discussed in#51651 (comment), when a datetime object is created from unix timestamp, the timezone constructor argument is ignored as demonstrated inhttps://onlinephp.io/c/b07d1 and also mentioned in [PHP documentation](https://www.php.net/manual/en/datetime.construct.php#refsect1-datetime.construct-parameters):> The $timezone parameter and the current timezone are ignored when the $datetime parameter either is a UNIX timestamp (e.g. `@946684800`) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00).This change shouldn't break any existing logic, given the places where this time is used already include timezone in the date format string.### Changes in effect```diff  ------------- ------------------------------------ ---------------------------   Message       Trigger                              Next Run  ------------- ------------------------------------ ----------------------------  TestMessage   PeriodicalTrigger: every 2 seconds   2023-09-16T15:54:46+00:00+  TestMessage   PeriodicalTrigger: every 2 seconds   2023-09-16T18:54:46+03:00  ------------- ------------------------------------ ---------------------------```Commits-------9baf427 Match next run timezone with from timezone

This was referencedOct 21, 2023

Release v6.4.0-BETA1#52220

Merged

Release v7.0.0-BETA1#52224

Merged