search

agronholm / apscheduler

Task scheduling library for Python
MIT License
6.3k stars 712 forks source link

Fix a daylight savings time issue in `CronTrigger` #981

Open hlobit opened 3 weeks ago

hlobit commented 3 weeks ago

Changes

Fixes #980.

agronholm commented 3 weeks ago

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

agronholm commented 3 weeks ago

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

agronholm commented 3 weeks ago

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 commented 3 weeks ago

Coverage Status

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

agronholm commented 1 week ago

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 commented 1 week ago

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 :heart: