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

different execution time behaviour for sec or min lowest field #616

Open
dllx opened this issue Feb 7, 2024 · 2 comments · May be fixed by #617
Open

different execution time behaviour for sec or min lowest field #616

dllx opened this issue Feb 7, 2024 · 2 comments · May be fixed by #617

Comments

@dllx
Copy link

dllx commented Feb 7, 2024

I'm experiencing different next execution time behaviour depending if the first field is seconds or minutes.
I think that without any explicit kind of configuration, the seconds field should not behave differently than the minutes field. It's an unexpected inconsistency.

Given

        CronDefinition cronDefinition = CronDefinitionBuilder.defineCron()
                .withMinutes().and()
                .withHours().and()
                .withDayOfMonth().and()
                .withMonth().and()
                .instance();
        Cron cron = new CronParser(cronDefinition).parse("0,1,3/1 * * * *");
        ExecutionTime executionTime = ExecutionTime.forCron(cron);

I expect the following, and indeed that's what I observe:

        ZonedDateTime now0 = ZonedDateTime.of(2024, 2, 7, 13, 0, 7, 123456789, ZoneOffset.UTC);
        ZonedDateTime expected0 = ZonedDateTime.of(2024, 2, 7, 13, 1, 0, 0, ZoneOffset.UTC);
        ZonedDateTime nextTime0 = executionTime.nextExecution(now0).get();
        assertEquals(expected0, nextTime0);

        ZonedDateTime now1 = ZonedDateTime.of(2024, 2, 7, 13, 1, 7, 123456789, ZoneOffset.UTC);
        ZonedDateTime expected1 = ZonedDateTime.of(2024, 2, 7, 13, 3, 0, 0, ZoneOffset.UTC);
        ZonedDateTime nextTime1 = executionTime.nextExecution(now1).get();
        assertEquals(expected1, nextTime1);

        ZonedDateTime now2 = ZonedDateTime.of(2024, 2, 7, 13, 2, 7, 123456789, ZoneOffset.UTC);
        ZonedDateTime expected2 = ZonedDateTime.of(2024, 2, 7, 13, 3, 0, 0, ZoneOffset.UTC);
        ZonedDateTime nextTime2 = executionTime.nextExecution(now2).get();
        assertEquals(expected2, nextTime2);

On the other hand, if I add a seconds field into the definition like

        CronDefinition cronDefinition = CronDefinitionBuilder.defineCron()
                .withSeconds().and()
                .withMinutes().and()
                .withHours().and()
                .withDayOfMonth().and()
                .withMonth().and()
                .instance();
        Cron cron = new CronParser(cronDefinition).parse("0,1,3/1 * * * * *");
        ExecutionTime executionTime = ExecutionTime.forCron(cron);

I expect the following, but the result is different if the seconds field matches and the next second would also match the seconds field! In that case, the nanos field is not set to zero.

        ZonedDateTime now0 = ZonedDateTime.of(2024, 2, 7, 13, 46, 0, 999999999, ZoneOffset.UTC);
        ZonedDateTime expected0 = ZonedDateTime.of(2024, 2, 7, 13, 46, 1, 0, ZoneOffset.UTC);
        ZonedDateTime nextTime0 = executionTime.nextExecution(now0).get();
        System.out.println(nextTime0); // 2024-02-07T13:46:01.999999999Z
        //assertEquals(expected0, nextTime0);

        ZonedDateTime now1 = ZonedDateTime.of(2024, 2, 7, 13, 46, 1, 123456789, ZoneOffset.UTC);
        ZonedDateTime expected1 = ZonedDateTime.of(2024, 2, 7, 13, 46, 3, 0, ZoneOffset.UTC);
        ZonedDateTime nextTime1 = executionTime.nextExecution(now1).get();
        assertEquals(expected1, nextTime1);

        ZonedDateTime now2 = ZonedDateTime.of(2024, 2, 7, 13, 46, 2, 123456789, ZoneOffset.UTC);
        ZonedDateTime expected2 = ZonedDateTime.of(2024, 2, 7, 13, 46, 3, 0, ZoneOffset.UTC);
        ZonedDateTime nextTime2 = executionTime.nextExecution(now2).get();
        assertEquals(expected2, nextTime2);

By the way, to possibly counter the "strange" expression 3/1 used above, the behaviour can also be seen when using a plain * for the leftmost cron field.
If it's "seconds", then the nano part of the nextExecution time is not set to zero, if it's "minutes", then the nano (and the second) part of the nextExecution time is indeed set to zero.

@dllx dllx linked a pull request Feb 8, 2024 that will close this issue
@jmrozanec
Copy link
Owner

@dllx thank you for reporting this and for contributing a fix! We will soon merge and release a new version! Thanks! 😎

@dllx
Copy link
Author

dllx commented Feb 9, 2024

There is also something similarly wrong in the ExecutionTime.isMatch method if the nanos are not zero.
I will provide a better test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants