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

Fix a daylight savings time issue in CronTrigger #981

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

hlobit
Copy link

@hlobit hlobit commented Oct 28, 2024

Changes

Fixes #980.

(1) remove the `timedelta` operations - which are not timezone aware
(2) make sure the "fold" attribute remains when incrementing
@agronholm
Copy link
Owner

How did you get the test to pass for you locally? Are the new tests sensitive to the local time zone?

@agronholm
Copy link
Owner

Either way, I'll take a look at this tomorrow.

@agronholm
Copy link
Owner

Oh, and I'll need a new changelog entry for this too (don't delete the pull request template, as it contains a check list of things for the PR to be accepted).

@coveralls
Copy link

Coverage Status

coverage: 91.996% (-0.01%) from 92.006%
when pulling 7455b52 on hlobit:cron-trigger-dst-fold-fix
into 8660590 on agronholm:master.

@agronholm
Copy link
Owner

Oops, it's been 2 weeks already! I got distracted by several other projects I'm working on. Anyways, the workings of the cron trigger are not fresh in my memory, so would you mind explaining these changes? They look fine, but I'd like to understand why these specific changes were necessary.

@hlobit
Copy link
Author

hlobit commented Nov 11, 2024

No problem !
I think what is key in this change is the replacement of a statement like

self._last_fire_time + timedelta(seconds=1)

by

datetime.fromtimestamp(
    self._last_fire_time.timestamp() + 1, self.timezone
)

For any reason, the first one does not take DST into account.
https://docs.python.org/3/library/datetime.html#datetime.datetime.fold

Note that no time zone adjustments are done even if the input is an aware object.

The second one does the trick because it is doing operations using the timestamp(), which handles the fold attribute.
https://docs.python.org/3/library/datetime.html#datetime.datetime.timestamp

The timestamp() method uses the fold attribute to disambiguate the times during a repeated interval.

The other change is to make sure the value of fold attribute is not lost while returning a new aware datetime object from _set_field_value().

This made my use cases pass, see the tests.

And thanks for the great apscheduler ❤️

Copy link
Owner

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

Nice work, but I'd like a few tweaks before I merge.

Comment on lines +437 to +438
assert next_date == correct_next_date.replace(tzinfo=timezone)
assert next_date.fold == fold
Copy link
Owner

Choose a reason for hiding this comment

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

This would make me feel better about the correctness.

Suggested change
assert next_date == correct_next_date.replace(tzinfo=timezone)
assert next_date.fold == fold
assert next_date == correct_next_date.replace(tzinfo=timezone, fold=fold)
assert str(next_date) == str(correct_next_date)

"cron_expression, start_time, correct_next_dates",
[
(
"0 * * * *",
Copy link
Owner

Choose a reason for hiding this comment

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

Why bother with from_crontab()? This could just be the minute field.

@@ -207,16 +207,17 @@ def _set_field_value(
else:
values[field.name] = new_value

return datetime(**values, tzinfo=self.timezone)
return datetime(**values, tzinfo=self.timezone).replace(fold=dateval.fold)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return datetime(**values, tzinfo=self.timezone).replace(fold=dateval.fold)
return datetime(**values, tzinfo=self.timezone, fold=dateval.fold)

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 this pull request may close these issues.

Bi-hourly CronTrigger runs into infinite loop at daylight savings time boundary
3 participants