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

Improved timezones handling in next_run #604

Merged
merged 15 commits into from
Nov 17, 2023

Conversation

SijmenHuizenga
Copy link
Collaborator

This improves how the timezone configured in .at("18:00", "Europe/Amsterdam") is processed.

The problem is that the self.next_run value is localized to self.at_time_zone and then converted to local time. In the next condition, the code uses datetime.datetime.now(), which fetches the current time in the local timezone. If the system's local timezone is different from self.at_time_zone, this can result in a discrepancy and cause the computation of self.next_run to be off by a day or more. This is what is happening in #603.

This PR changes the timezone-handling approach to use localized datetimes throughout the whole _schedule_next_run function.

PR created as draft since there are still a few untested cases that I would like to add tests for before merging. Especially in the area of combining .at() with weekdays, until() and daylight-saving-time.

@coveralls
Copy link

coveralls commented Oct 23, 2023

Coverage Status

coverage: 99.474% (-0.3%) from 99.726%
when pulling 418693b on SijmenHuizenga:timezone-fix-simplify
into 073dbc6 on dbader:master.

@sosdarko
Copy link

I had a similar problem, but kind of opposite... I use every(day).at(...) and UTC times, but my machine is in different time zone.
When I want to test if execution of the job is correct, I put some close time as scheduled time, but due to this bug, it is scheduled for next day. I found the reason and modify code in _schedule_next_run so it takes into account time zone set when schedule was defined. That's around lines where you ask is it the first run (last_run is None), and compare self.at_time with now.time(). This comparison doesn't take into account time zone.

This is important not only for testing, but for restarts, because with this bug, when I restart process that runs schedule, I miss some of the scheduled executions. @SijmenHuizenga, thanks for rectifying this, hope that next version will be even better :)

@SijmenHuizenga
Copy link
Collaborator Author

Thank you @sosdarko for explaining your use-case! I'm also looking forward to releasing a version that resolves this bug.

Before I go forward with merging this, I want to be sure this fix resolves the the issues. Would you be willing to help test the fix in this pr? Either by running it as part of your program, or by writing a unit-test like the these ones that represents your situation?

@sosdarko
Copy link

Hi @SijmenHuizenga , yes I can do both, should I take your last commit? Is it this one: e7e8be6

@SijmenHuizenga
Copy link
Collaborator Author

@sosdarko Amazing! Yes, the last one is the one to take: 51d762e

@sosdarko
Copy link

next_time is calculated right, but in execution I got this:

init.py", line 768, in _schedule_next_run
if not self.last_run or (self.next_run - self.last_run) > self.period:
~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
TypeError: can't subtract offset-naive and offset-aware datetimes

@SijmenHuizenga
Copy link
Collaborator Author

@sosdarko good find, the unit tests apparently do not cover timezone usage with multiple runs 😓. Will fix this later today or tomorrow.

@SijmenHuizenga SijmenHuizenga marked this pull request as ready for review November 12, 2023 15:28
@SijmenHuizenga SijmenHuizenga merged commit a388620 into dbader:master Nov 17, 2023
15 checks passed
@SijmenHuizenga SijmenHuizenga deleted the timezone-fix-simplify branch November 17, 2023 22:04
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.

3 participants