Skip to content
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

quarkus-scheduler trigger getNextFireTime does not consider cron timezone #41717

Closed
alanmscott opened this issue Jul 5, 2024 · 10 comments · Fixed by #41778
Closed

quarkus-scheduler trigger getNextFireTime does not consider cron timezone #41717

alanmscott opened this issue Jul 5, 2024 · 10 comments · Fixed by #41778
Assignees
Labels
area/scheduler kind/bug Something isn't working
Milestone

Comments

@alanmscott
Copy link

Describe the bug

When using the Quarkus Scheduler, cron triggering works properly with respect to the configured timezone. However, when interrogating the scheduler for the "getNextFireTime", the timezone is not factored in. This results in incorrect behaviour, most problematically, is that it can return a next fire time in the past.

See example class which demonstrates the results coming back.

Expected behavior

When calling getNextFireTime from the scheduler, the Instant returned should match the actual nextFireTime defined by the cron AND timezone, not simply the cron as parsed in the local timezone.

Actual behavior

When calling getNextFireTime from the scheduler, the Instant returned treats the cron as if it had the local timezone.

How to Reproduce?

package org.example;

import java.time.Instant;

import io.quarkus.runtime.StartupEvent;
import io.quarkus.scheduler.Scheduler;
import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.event.Observes;
import jakarta.inject.Inject;

@ApplicationScoped
public class TestScheduler {

  @Inject
  Scheduler scheduler;

  public void init(@Observes StartupEvent startupEvent) {

    scheduler.newJob("testJob1")
             .setCron("0 30 20 ? * * *")
             .setTimeZone("Europe/Berlin")
             .setTask(execution -> System.out.println("Job has run"))
             .schedule();

    scheduler.newJob("testJob2")
             .setCron("0 30 20 ? * * *")
             .setTimeZone("Europe/London")
             .setTask(execution -> System.out.println("Job has run"))
             .schedule();

    Instant nextFireTime1 = scheduler.getScheduledJob("testJob1").getNextFireTime();
    Instant nextFireTime2 = scheduler.getScheduledJob("testJob2").getNextFireTime();

    System.out.println("=============================================================");
    System.out.println("NextFireTimes:");
    System.out.println("TestJob1(Berlin)=" + nextFireTime1);
    System.out.println("TestJob2(London)=" + nextFireTime2);
    System.out.println("compareTo result=" + nextFireTime1.compareTo(nextFireTime2) + "    <-- should be greater than 0");
    System.out.println("=============================================================");
    // These two times should be different as they are crons with same local time but diff timezone, so UTC time expressed by the Instant should be different.
  }

}

Output of uname -a or ver

Linux alan-test-redis-cacher-56d9f6f6d4-ssstk 5.4.0-42-generic #46-Ubuntu SMP Fri Jul 10 00:24:02 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Output of java -version

openjdk version "21.0.1" 2023-10-17 LTS

Quarkus version or git rev

3.10.0

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@alanmscott alanmscott added the kind/bug Something isn't working label Jul 5, 2024
Copy link

quarkus-bot bot commented Jul 5, 2024

/cc @manovotn (scheduler), @mkouba (scheduler)

@mkouba
Copy link
Contributor

mkouba commented Jul 8, 2024

The Trigger#getNextFireTime() method and others (i.e. ScheduledExecution#getFireTime()) intentionally return an Instant which lacks the timezone info. If you need to work with a specific timezone then use the ZonedDateTime returned from the Instant#atZone(ZoneId).

@alanmscott
Copy link
Author

alanmscott commented Jul 8, 2024

Hi @mkouba - thanks for chipping in.
Sadly the Instant being returned does not match the actual instant that the job will next run at though.

I tried putting in a work around by returning the Instant and then forcing it into the timezone, using the methods you mention, but that has the consequence that it can return "nextFireTime" of an event that has already triggered (i.e. in the past).

As the scheduled job has the capability of defining a timezone, then that must be considered when returning a timezone agnostic Instant. The Instant of 20:30 London is not the same as the Instant of 20:30 Berlin.

@mkouba
Copy link
Contributor

mkouba commented Jul 8, 2024

The Instant of 20:30 London is not the same as the Instant of 20:30 Berlin.

Of course, it isn't. And I agree that it's confusing.

It works like this - all Instants are obtained from the default time zone. Also all triggers are compared to now() which is obtained from the default time zone. For example, on my machine the default timezone is Europe/Prague and the next fire time for the cron 0 30 20 ? * * * would be something like 2024-07-08T18:30:00Z.

Now if I have 0 30 20 ? * * * for Europe/Prague then getNextFireTime().atZone(ZoneId.of("Europe/Prague") returns 2024-07-08T20:30+02:00[Europe/Prague]. And 0 30 20 ? * * * for Europe/London returns 2024-07-08T19:30+01:00[Europe/London]. Which is correct because it should be triggered one hour earlier.

In your code, nextFireTime1.compareTo(nextFireTime2) should return 1 because testJob1 should be executed after testJob2. And it does return 1 on my machine.

Does that make sense? Would it help if we clarify the javadoc in a way that getNextFireTime() returns an Instant obtained from the default time zone?

@alanmscott
Copy link
Author

alanmscott commented Jul 8, 2024

Hi,

Thanks for sticking with me!

And 0 30 20 ? * * * for Europe/London returns 2024-07-08T19:30+01:00[Europe/London]. Which is correct because it should be triggered one hour earlier.

Shouldn't this return 2024-07-08T20:30+01:00[Europe/London] if the cron has been set to 0 30 20 with a timezone of Europe/London?

I always get the same Instant for both cron schedules, so the compareTo always returns 0.

Out of interest, if you try the same experiment, using a cron schedule of 10-15mins in your future with a more easterly timezone, e.g. Europe/Istantbul, do you get a value for today? Or is it correctly recognising that the trigger actually will next take place tomorrow?
In my experiments, it still returns a value from today, even though that time has passed in the cron target timezone.

In case it helps, here is my output:

Expanded test class:

package org.example;

import java.time.Instant;
import java.time.ZoneId;
import java.time.format.DateTimeFormatter;

import io.quarkus.runtime.StartupEvent;
import io.quarkus.scheduler.Scheduler;
import jakarta.enterprise.context.ApplicationScoped;
import jakarta.enterprise.event.Observes;
import jakarta.inject.Inject;

@ApplicationScoped
public class TestScheduler {

  @Inject
  Scheduler scheduler;

  public void init(@Observes StartupEvent startupEvent) {

    String localCron = "0 30 15 ? * * *";

    scheduler.newJob("testJob1")
             .setTimeZone("UTC")
             .setCron(localCron)
             .setTask(execution -> System.out.println("Job1 has run"))
             .schedule();

    scheduler.newJob("testJob2")
             .setTimeZone("Europe/London")
             .setCron(localCron)
             .setTask(execution -> System.out.println("Job2 has run"))
             .schedule();

    scheduler.newJob("testJob3")
             .setTimeZone("Europe/Berlin")
             .setCron(localCron)
             .setTask(execution -> System.out.println("Job3 has run"))
             .schedule();

    scheduler.newJob("testJob4")
             .setTimeZone("Europe/Istanbul")
             .setCron(localCron)
             .setTask(execution -> System.out.println("Job4 has run"))
             .schedule();

    Instant nextFireTime1 = scheduler.getScheduledJob("testJob1").getNextFireTime();
    Instant nextFireTime2 = scheduler.getScheduledJob("testJob2").getNextFireTime();
    Instant nextFireTime3 = scheduler.getScheduledJob("testJob3").getNextFireTime();
    Instant nextFireTime4 = scheduler.getScheduledJob("testJob4").getNextFireTime();

    System.out.println("=============================================================");
    System.out.println("NextFireTimes:");
    System.out.println("TestJob1(UTC     )=" + nextFireTime1 + " / " + nextFireTime1.atZone(ZoneId.of("UTC")).format(DateTimeFormatter.ISO_ZONED_DATE_TIME));
    System.out.println("TestJob2(London  )=" + nextFireTime2 + " / " + nextFireTime1.atZone(ZoneId.of("Europe/London")).format(DateTimeFormatter.ISO_ZONED_DATE_TIME));
    System.out.println("TestJob3(Berlin  )=" + nextFireTime3 + " / " + nextFireTime1.atZone(ZoneId.of("Europe/Berlin")).format(DateTimeFormatter.ISO_ZONED_DATE_TIME));
    System.out.println("TestJob4(Istanbul)=" + nextFireTime4 + " / " + nextFireTime1.atZone(ZoneId.of("Europe/Istanbul")).format(DateTimeFormatter.ISO_ZONED_DATE_TIME));
    System.out.println("compareTo result1-2=" + nextFireTime1.compareTo(nextFireTime2) + "    <-- should be greater than 0");
    System.out.println("compareTo result2-3=" + nextFireTime2.compareTo(nextFireTime3) + "    <-- should be greater than 0");
    System.out.println("compareTo result3-4=" + nextFireTime3.compareTo(nextFireTime4) + "    <-- should be greater than 0");
    System.out.println("=============================================================");
    // These two times should be different as they are crons with same local time but diff timezone, so UTC time should be different.
  }


}

OUTPUT:

=============================================================
NextFireTimes:
TestJob1(UTC     )=2024-07-08T14:30:00Z / 2024-07-08T14:30:00Z[UTC]
TestJob2(London  )=2024-07-08T14:30:00Z / 2024-07-08T15:30:00+01:00[Europe/London]
TestJob3(Berlin  )=2024-07-08T14:30:00Z / 2024-07-08T16:30:00+02:00[Europe/Berlin]
TestJob4(Istanbul)=2024-07-08T14:30:00Z / 2024-07-08T17:30:00+03:00[Europe/Istanbul]
compareTo result1-2=0    <-- should be greater than 0
compareTo result2-3=0    <-- should be greater than 0
compareTo result3-4=0    <-- should be greater than 0
=============================================================

Expected result:

TestJob1(UTC     )=2024-07-08T15:30:00Z / 2024-07-08T15:30:00Z[UTC]
TestJob2(London  )=2024-07-08T14:30:00Z / 2024-07-08T15:30:00+01:00[Europe/London]       <-- same day as 15:30 London has not yet passed 
TestJob3(Berlin  )=2024-07-09T13:30:00Z / 2024-07-08T15:30:00+02:00[Europe/Berlin]        <-- next day as 15:30 Berlin has passed already
TestJob4(Istanbul)=2024-07-09T12:30:00Z / 2024-07-08T15:30:00+03:00[Europe/Istanbul]       <-- next day as 15:30 Istanbul has passed already

@alanmscott
Copy link
Author

And just to clarify, the triggering itself is working fine. The jobs are only actually triggered at the expected times.

@mkouba
Copy link
Contributor

mkouba commented Jul 8, 2024

And 0 30 20 ? * * * for Europe/London returns 2024-07-08T19:30+01:00[Europe/London]. Which is correct because it should be triggered one hour earlier.

Shouldn't this return 2024-07-08T20:30+01:00[Europe/London] if the cron has been set to 0 30 20 with a timezone of Europe/London?

Not really. I've tried to explain in my previous comment that the next fire time is always derived from the default time zone. So if you have a cron 0 30 20 ? * * * and the default time zone is Europe/Berlin then no matter what the timezone attribute is the next fire time is an Instant derived from yyyy-MM-ddT20:30+02:00[Europe/Berlin].

I always get the same Instant for both cron schedules, so the compareTo always returns 0.

That is expected, although not very clear.

I will try to improve the wording of the javadoc and send a PR tomorrow.

@alanmscott
Copy link
Author

OK, but that's not the correct value for NextFireTime then, surely?
If it's always returning the same Instant, regardless of the cron timezone, and the timezone DOES change the actual cron trigger time, then the getNextFireTime is not correct.

This example showing a trigger highlights it.
If the NextFireTime Instant was correct, then all 4 would be expected to trigger simultaneously, but instead, only the London one does, which is correct from a triggering point of view.

2024-07-08 15:33:30,858 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) =============================================================
2024-07-08 15:33:30,863 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) NextFireTimes:
2024-07-08 15:33:30,866 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) TestJob1(UTC     )=2024-07-08T14:35:00Z / 2024-07-08T14:35:00Z[UTC]
2024-07-08 15:33:30,866 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) TestJob2(London  )=2024-07-08T14:35:00Z / 2024-07-08T15:35:00+01:00[Europe/London]
2024-07-08 15:33:30,867 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) TestJob3(Berlin  )=2024-07-08T14:35:00Z / 2024-07-08T16:35:00+02:00[Europe/Berlin]
2024-07-08 15:33:30,867 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) TestJob4(Istanbul)=2024-07-08T14:35:00Z / 2024-07-08T17:35:00+03:00[Europe/Istanbul]
2024-07-08 15:33:30,867 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) compareTo result1-2=0     <-- should be greater than 0
2024-07-08 15:33:30,867 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) compareTo result2-3=0     <-- should be greater than 0
2024-07-08 15:33:30,868 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) compareTo result3-4=0     <-- should be greater than 0
2024-07-08 15:33:30,868 INFO  [org.exa.TestScheduler] (Quarkus Main Thread) =============================================================
...
2024-07-08 15:35:01,003 INFO  [org.exa.TestScheduler] (vert.x-worker-thread-1) Job2 has run

@mkouba
Copy link
Contributor

mkouba commented Jul 8, 2024

This example showing a trigger highlights it.
If the NextFireTime Instant was correct, then all 4 would be expected to trigger simultaneously, but instead, only the London one does, which is correct from a triggering point of view.

That's a good point and indeed it does not make a lot of sense from this point of view.

So I tested the quarkus-quartz extension and it seems that getNextFireTime() returns the Instant derived from the zoned next fire time.

I will send a PR tomorrow. We should be consistent here.

@mkouba mkouba self-assigned this Jul 8, 2024
@alanmscott
Copy link
Author

Thanks @mkouba - really appreciate your persistence.

mkouba added a commit to mkouba/quarkus that referenced this issue Jul 9, 2024
mkouba added a commit to mkouba/quarkus that referenced this issue Jul 9, 2024
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 10, 2024
@gsmet gsmet modified the milestones: 3.13 - main, 3.12.3 Jul 16, 2024
gsmet pushed a commit to gsmet/quarkus that referenced this issue Jul 16, 2024
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/scheduler kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants