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: replace loop timeout by max match date #686

Merged
merged 2 commits into from
Aug 14, 2023

Conversation

sheerlox
Copy link
Collaborator

@sheerlox sheerlox commented Aug 14, 2023

Description

This change is inspired by cronie's crond.
It replaces the 5 seconds timeout in the _getNextDateFrom while loop by a maximum date in the future (8 years).

Related Issue

Fixes #467.
Closes #499.

Motivation and Context

In environments with limited resources (CPU/memory), it may take more than 5 seconds to find the next date. This throws an error (see #467), while it should only take the time it needs and find the occurence.

How Has This Been Tested?

Using the systemd-run command, I've been able to compare the behavior of test runs between the main branch and this version and to confirm that these changes have all tests passing correctly in a low resources environment.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • If my change introduces a breaking change, I have added a ! after the type/scope in the title (see the Conventional Commits standard).

This issue was initially discussed on Discord.

@sheerlox sheerlox self-assigned this Aug 14, 2023
@sheerlox sheerlox marked this pull request as ready for review August 14, 2023 18:14
@intcreator
Copy link
Collaborator

intcreator commented Aug 14, 2023

for posterity: @sheerlox and I had a conversation about this on Discord. we decided that it's ok to take a long time to find the next cron execution time since that could be up to 8 years away. if it takes more than a few seconds to find the next execution time then the execution time is almost certainly farther away than how long it takes to find it (for example if at 8:00:00 pm you look for the next execution time and it takes you until 8:00:08 it's probably because the next execution time is hours, days, months, or years away and it won't cause errors to take so long finding it).

we feel good closing #467 because this will remove the 5 second timeout (which we decided isn't needed) and also #499 which would have added a parameter to bypass the 5 second timeout.

the ~8 year check should in theory never trigger but if it does it will alert us that something is wrong in the code since no cron execution time should ever be more than 8 years away by definition from the spec

@sheerlox sheerlox merged commit c685c63 into kelektiv:main Aug 14, 2023
13 checks passed
@sheerlox sheerlox deleted the fix/467/remove-loop-limit branch August 14, 2023 22:15
ncb000gt pushed a commit that referenced this pull request Aug 14, 2023
## [2.4.1](v2.4.0...v2.4.1) (2023-08-14)

### 🐛 Bug Fixes

* replace loop timeout by max match date ([#686](#686)) ([c685c63](c685c63))

### ⚙️ Continuous Integrations

* **renovate:** configure renovate ([#683](#683)) ([9dbe962](9dbe962))
@ncb000gt
Copy link
Member

🎉 This PR is included in version 2.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

ncb000gt pushed a commit that referenced this pull request Aug 15, 2023
## [3.0.0-beta.2](v3.0.0-beta.1...v3.0.0-beta.2) (2023-08-15)

### 🐛 Bug Fixes

* replace loop timeout by max match date ([#686](#686)) ([c685c63](c685c63))

### 🚨 Tests

* update new test for cron standard alignments ([ea56fc1](ea56fc1))

### ⚙️ Continuous Integrations

* **renovate:** configure renovate ([#683](#683)) ([9dbe962](9dbe962))
@ncb000gt
Copy link
Member

🎉 This PR is included in version 3.0.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

patrickm68 added a commit to patrickm68/node-cron-types that referenced this pull request Sep 14, 2023
## [2.4.1](kelektiv/node-cron@v2.4.0...v2.4.1) (2023-08-14)

### 🐛 Bug Fixes

* replace loop timeout by max match date ([#686](kelektiv/node-cron#686)) ([c685c63](kelektiv/node-cron@c685c63))

### ⚙️ Continuous Integrations

* **renovate:** configure renovate ([#683](kelektiv/node-cron#683)) ([9dbe962](kelektiv/node-cron@9dbe962))
sheerlox pushed a commit that referenced this pull request Sep 27, 2023
## [2.4.1](v2.4.0...v2.4.1) (2023-08-14)

### 🐛 Bug Fixes

* replace loop timeout by max match date ([#686](#686)) ([c685c63](c685c63))

### ⚙️ Continuous Integrations

* **renovate:** configure renovate ([#683](#683)) ([9dbe962](9dbe962))
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 27, 2023
sheerlox pushed a commit to sheerlox/node-cron that referenced this pull request Sep 27, 2023
## [2.4.1](kelektiv/node-cron@v2.4.0...v2.4.1) (2023-08-14)

### 🐛 Bug Fixes

* replace loop timeout by max match date ([kelektiv#686](kelektiv#686)) ([c685c63](kelektiv@c685c63))

### ⚙️ Continuous Integrations

* **renovate:** configure renovate ([kelektiv#683](kelektiv#683)) ([9dbe962](kelektiv@9dbe962))
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
sheerlox pushed a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
## [2.4.1](kelektiv/node-cron@v2.4.0...v2.4.1) (2023-08-14)

### 🐛 Bug Fixes

* replace loop timeout by max match date ([kelektiv#686](kelektiv#686)) ([c685c63](kelektiv@c685c63))

### ⚙️ Continuous Integrations

* **renovate:** configure renovate ([kelektiv#683](kelektiv#683)) ([9dbe962](kelektiv@9dbe962))
sheerlox added a commit to sheerlox/node-cron that referenced this pull request Sep 29, 2023
@sheerlox sheerlox added type:bug Bug reports and bug fixes and removed released on @beta labels Sep 30, 2023
@sheerlox sheerlox removed their assignment Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released type:bug Bug reports and bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Something went wrong. cron reached maximum iterations.
3 participants