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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/versionhistory.rst
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ APScheduler, see the :doc:`migration section <migration>`.
acquire the same schedules at once
- Changed ``SQLAlchemyDataStore`` to automatically create the explicitly specified
schema if it's missing (PR by @zhu0629)
- Fixed an issue with ``CronTrigger`` infinitely looping to get next date when DST ends
(`#980 <https://github.com/agronholm/apscheduler/issues/980>`_; PR by @hlobit)

**4.0.0a5**

Expand Down
19 changes: 6 additions & 13 deletions src/apscheduler/triggers/cron/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from __future__ import annotations

from collections.abc import Sequence
from datetime import datetime, timedelta, tzinfo
from datetime import datetime, tzinfo
from typing import Any, ClassVar

import attrs
Expand Down Expand Up @@ -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)


def next(self) -> datetime | None:
if self._last_fire_time:
start_time = self._last_fire_time + timedelta(microseconds=1)
next_time = datetime.fromtimestamp(
self._last_fire_time.timestamp() + 1, self.timezone
)
else:
start_time = self.start_time
next_time = self.start_time

fieldnum = 0
next_time = datetime_ceil(start_time).astimezone(self.timezone)
while 0 <= fieldnum < len(self._fields):
field = self._fields[fieldnum]
curr_value = field.get_value(next_time)
Expand Down Expand Up @@ -276,11 +277,3 @@ def __repr__(self) -> str:

fields.append(f"timezone={timezone_repr(self.timezone)!r}")
return f'CronTrigger({", ".join(fields)})'


def datetime_ceil(dateval: datetime) -> datetime:
"""Round the given datetime object upwards."""
if dateval.microsecond > 0:
return dateval + timedelta(seconds=1, microseconds=-dateval.microsecond)

return dateval
38 changes: 38 additions & 0 deletions tests/triggers/test_cron.py
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,44 @@ def test_dst_change(
)


@pytest.mark.parametrize(
"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.

datetime(2024, 10, 27, 2, 0, 0, 0),
[
(datetime(2024, 10, 27, 2, 0, 0, 0), 0),
(datetime(2024, 10, 27, 2, 0, 0, 0), 1),
(datetime(2024, 10, 27, 3, 0, 0, 0), 0),
],
),
(
"1 * * * *",
datetime(2024, 10, 27, 2, 1, 0, 0),
[
(datetime(2024, 10, 27, 2, 1, 0, 0), 0),
(datetime(2024, 10, 27, 2, 1, 0, 0), 1),
(datetime(2024, 10, 27, 3, 1, 0, 0), 0),
],
),
],
ids=["dst_change_0", "dst_change_1"],
)
def test_dst_change2(
cron_expression,
start_time,
correct_next_dates,
timezone,
):
trigger = CronTrigger.from_crontab(cron_expression, timezone=timezone)
trigger.start_time = start_time.replace(tzinfo=timezone)
for correct_next_date, fold in correct_next_dates:
next_date = trigger.next()
assert next_date == correct_next_date.replace(tzinfo=timezone)
assert next_date.fold == fold
Comment on lines +437 to +438
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)



def test_zero_value(timezone):
start_time = datetime(2020, 1, 1, tzinfo=timezone)
trigger = CronTrigger(
Expand Down