Handle DST better #317

Merged
merged 2 commits into from Dec 20, 2016

Conversation

Projects
None yet
3 participants
@mj1856
Contributor

mj1856 commented Dec 23, 2015

This PR addresses the challenges of scheduling a daily event when considering daylight saving time transitions.

First, it's worth pointing out that the Quartz.net documentation's description of how DST is handled is wrong. It states multiple times (in the FAQ here, in the CronTrigger Tutorial here, in the Best Practices here, and probably in the API docs somewhere) that an occurrence in the spring-forward gap could be skipped and an occurrence in the fall-back overlap could execute twice. This is not actually the current implemented behavior.

The current implemented behavior is actually:

  • In the spring, when a local time doesn't exist, run at the same moment in time as if the transition did not occur.
  • In the fall, when two occurrences exist with the same local time, run at the standard time occurrence.

This behavior is ultimately coming from TimeZoneInfo.GetUtcOffset, which always returns the standard time offset when in doubt.

While the spring behavior is fine, unfortunately the fall behavior is usually undesired in scheduling systems. When there are two occurrences of the same local time, a daily event should be expected to run at the first occurrence encountered, since time progresses in a forward direction. The first occurrence corresponds to the daylight time, not the standard time.

This PR addresses this by adding an overload of Quartz.Util.TimeZoneUtil.GetUtcOffset that accepts a DateTime, and has the desired implementation. Then throughout the code, anywhere that TimeZoneInfo.GetUtcOffset was used, this new method is used instead.

Unit tests are included, and all existing unit tests pass.

The documentation should also be updated to reflect the actual behavior. Thanks.

@drusellers

This comment has been minimized.

Show comment
Hide comment

👏

@mj1856

This comment has been minimized.

Show comment
Hide comment
@mj1856

mj1856 Dec 19, 2016

Contributor

You know, it's been almost a year, and I forgot all about this.

I will rebase it to the current master shortly. Is there any other reason this wasn't merged previously?

Contributor

mj1856 commented Dec 19, 2016

You know, it's been almost a year, and I forgot all about this.

I will rebase it to the current master shortly. Is there any other reason this wasn't merged previously?

@lahma

This comment has been minimized.

Show comment
Hide comment
@lahma

lahma Dec 20, 2016

Member

I guess this PR flew under the radar for me somehow, so I'm to blame. The master now is ongoing work for 3.0 (now in alpha). I guess this would be quite safe to be included in 2.x maintenance branch. I can backport this unless you are willing to throw yet another pull request for the 2.x branch.

Member

lahma commented Dec 20, 2016

I guess this PR flew under the radar for me somehow, so I'm to blame. The master now is ongoing work for 3.0 (now in alpha). I guess this would be quite safe to be included in 2.x maintenance branch. I can backport this unless you are willing to throw yet another pull request for the 2.x branch.

@lahma lahma merged commit 4d06bf2 into quartznet:master Dec 20, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@lahma

This comment has been minimized.

Show comment
Hide comment
@lahma

lahma Dec 20, 2016

Member

Excellent PR, thank you for taking the time to prepare it with test cases included. Now part of 3.0 deliverables.

Member

lahma commented Dec 20, 2016

Excellent PR, thank you for taking the time to prepare it with test cases included. Now part of 3.0 deliverables.

@drusellers

This comment has been minimized.

Show comment
Hide comment
@drusellers

drusellers Dec 20, 2016

SWEET! Looking forward to 3.0

SWEET! Looking forward to 3.0

@mj1856

This comment has been minimized.

Show comment
Hide comment
@mj1856

mj1856 Dec 20, 2016

Contributor

Thanks. I'll leave the backporting to you. I don't have an active project that uses Quartz.net at the moment, just I recommend it a lot. ;)

Contributor

mj1856 commented Dec 20, 2016

Thanks. I'll leave the backporting to you. I don't have an active project that uses Quartz.net at the moment, just I recommend it a lot. ;)

@mj1856

This comment has been minimized.

Show comment
Hide comment
@mj1856

mj1856 Dec 20, 2016

Contributor

Also, reminder - you'll want to update the doc locations I pointed out originally, and any others that describe DST behavior. With this change, the rules are fixed to:

  • Pick the earlier (daylight) occurrence when ambiguous. (1:30 PDT, not 1:30 PST)
  • Skip by the gap duration (usually 1h) when invalid. (2:30 PST => 3:30 PDT)
Contributor

mj1856 commented Dec 20, 2016

Also, reminder - you'll want to update the doc locations I pointed out originally, and any others that describe DST behavior. With this change, the rules are fixed to:

  • Pick the earlier (daylight) occurrence when ambiguous. (1:30 PDT, not 1:30 PST)
  • Skip by the gap duration (usually 1h) when invalid. (2:30 PST => 3:30 PDT)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment