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

build(schedule): replace dependency cron with croner #1165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hexagon
Copy link

@Hexagon Hexagon commented Feb 12, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Build related changes

What is the current behavior?

Issue Number: #1164

What is the new behavior?

This PR replaces dependency cron with croner.

Croner

Changes and new features brought by this PR

  • Now uses standard syntax for specifying months. Previously, it expected months to be represented as 0-11, but now it expects the "standard" 1-12 format like Vixie-cron, crontab.guru, etc.
  • Supports using L in the pattern to indicate the last day of the month or the last weekday of the month.
  • Supports nth weekday of month, including last weekday of month. Example: 5#L is last friday of current month.
  • Adds an option legacyMode: Which adds support for using an alternative way of combining the day of the week and day of the month - to be able to find, for example, Friday the 13th. More information on this feature is available in croner issue #53.
  • Adds an option context: that allows the user to pass a context (object) to the triggered function.
  • Adds an option overrunProtection: that allows the user to block executions that would overlap the previous execution.
  • Adds an option maxRuns: to specify the maximum number of executions before stopping the job.
  • Adds an option interval: to specify the minimum number of seconds between triggers, regardless of how the pattern looks.
  • Removes the dependency on Luxon, making the package much smaller.

Does this PR introduce a breaking change?

  • Yes
  • Type for option utcOffset changes from string | number to number
  • Code will now throw if trying to combine utcOffset with timezone, as this combination does not make sense.
  • Croner methods .previousRun(), .nextRun() and .nextRuns() return native JavaScript Date objects instead of moment-objects.
  • Job instances of cron and croner differ in exported members. Documentation need to be updated
    
    stop() - stops a job that is scheduled to run.
    start() - restarts a job that has been stopped.
    setTime(time: CronTime) - stops a job, sets a new time for it, and then starts it
    lastDate() - returns a string representation of the last date a job executed
    nextDates(count: number) - returns an array (size count) of moment objects representing upcoming job execution dates.
    HINT
    Use toDate() on moment objects to render them in human readable form.
    
    Should be changed to something like
    stop() - stops a job permanently and clears the internal timeOut, allowing the process to exit.
    pause() - pause a job that is scheduled to run.
    resume() - resumes a job that has been paused.
    currentRun() - returns a date object representing the date of the current run, this is updated before execution
    previousRun() - returns a date object representing the date of the previous run, this is updated after execution
    nextRuns(count: number) - returns an array (size count) of moment objects representing upcoming job execution dates.
    nextRun() - returns a Date object representing time of upcoming job execution.
    isStopped() - returns a boolean showing if the job is permanently stopped
    isRunning() - returns a boolean showing if the job is scheduled to run
    isBusy() - returns a boolean showing if the job is currently busy running a task 
    getPatter() - returns the original pattern
    HINT
    Use toLocaleString() on Date objects to render them in human readable form.
    
  • (because of above, some of the examples in the documentation need to be updated)
  • Will now support modifier L in pattern, needs to be added in documentation. From the croner documentation
    • L: L can be used in the day of the month field to specify the last day of the month. It can also be used in the day of the week field to specify the last specific weekday of the month, for example, the last Friday (`5#L`).
  • Adds new options, as mentioned earlier.

Other information

@Hexagon Hexagon marked this pull request as ready for review February 12, 2023 23:55
@Hexagon Hexagon force-pushed the swap-cron-croner branch 4 times, most recently from 35dd905 to db453ca Compare February 13, 2023 20:50
@Hexagon
Copy link
Author

Hexagon commented Feb 13, 2023

Made some changes to both Croner and the PR, should be ready for review now. If you're interested of course 🤓

@Hexagon Hexagon force-pushed the swap-cron-croner branch 3 times, most recently from 6697a02 to e90c70f Compare February 20, 2023 11:16
@Hexagon
Copy link
Author

Hexagon commented Mar 5, 2023

  • Updated for major version 6 of croner
  • Implemented most of available optionss from croner in nestjs/schedule
{
  preventOverrun?: boolean;
  interval?: number;
  context?: unknown;
  maxRuns?: number;
  legacyMode?: boolean;
}

@Hexagon
Copy link
Author

Hexagon commented Jun 22, 2023

Regarding #1296 , overrun protection is already supported by croner

@AbdlrahmanSaberAbdo
Copy link

Hi @kamilmysliwiec @Hexagon, what is the status of this PR? The current situation with node-cron is concerning due to its a good number critical issues, such as running jobs twice, and the fact that it has remained open for three years. In light of these issues, I believe it would be beneficial to consider using croner instead. I'm happy to contribute if there's anything
that still needs to be done.

@Hexagon
Copy link
Author

Hexagon commented Jul 19, 2023

@AbdlrahmanSaberAbdo, this PR is still good to go on my end. I've released a few patches of Croner since this PR, but it is just to update the version number after merging.

@Hexagon
Copy link
Author

Hexagon commented Oct 9, 2023

Adding a comparison of various alternatives since the release of cron@3 and PR #1432

cron do pass most test by now, and do align with standards regarding month, but is slow, and seem to lack features like L (last day of month) and nth day of week (5,6#L = last sunday and saturday of month).

Croner is fastest, has most features, and have been stable with current feature set for quite a while.


codespace@codespaces-00f3ec:/workspaces/cron-comparison main [!]> npm run test

cron-comparison@1.4.0 benchmark
node --no-warnings src/benchmark.js

Tests performed at 2023-10-09T19:35:28.895Z

Tested libraries (npm trends):

Pattern 0 0 0 L 2 *

Tests

cron            - FAIL  - Error: Unknown alias: l
croner          - OK    - 2024-02-29 00:00:00
cronosjs        - OK    - 2024-02-29 00:00:00
node-cron       - FAIL  - Error: L is a invalid expression for day of month
node-schedule   - OK    - 2024-02-29 00:00:00

Benchmark (only OK)

croner          x 150,794 ops/sec ±8.96% (85 runs sampled)
cronosjs        x 66,798 ops/sec ±15.04% (87 runs sampled)
node-schedule   x 380 ops/sec ±9.81% (83 runs sampled)

Fastest is croner         

Pattern 0 0 0 * 2 5#L

Tests

cron            - FAIL  - Error: Unknown alias: l
croner          - OK    - 2024-02-23 00:00:00
cronosjs        - FAIL  - Error: Field item invalid format
node-cron       - FAIL  - 1970-01-01 00:00:00
node-schedule   - FAIL  - TypeError: Cannot read properties of null (reading 'nextInvocation')

Benchmark (only OK)

croner          x 59,777 ops/sec ±3.36% (88 runs sampled)

Fastest is croner         

Pattern 0 0 0 * 12 5#2

Tests

cron            - FAIL  - Error: Field (dayOfWeek) cannot be parsed
croner          - OK    - 2023-12-08 00:00:00
cronosjs        - OK    - 2023-12-08 00:00:00
node-cron       - FAIL  - 1970-01-01 00:00:00
node-schedule   - OK    - 2023-12-08 00:00:00

Benchmark (only OK)

croner          x 133,599 ops/sec ±5.63% (87 runs sampled)
cronosjs        x 58,524 ops/sec ±13.08% (87 runs sampled)
node-schedule   x 3,727 ops/sec ±4.23% (86 runs sampled)

Fastest is croner         

Pattern 0 0 0 L 12 5-6#3

Tests

cron            - FAIL  - Error: Unknown alias: l
croner          - OK    - 2023-12-15 00:00:00
cronosjs        - OK    - 2023-12-15 00:00:00
node-cron       - FAIL  - Error: L is a invalid expression for day of month
node-schedule   - FAIL  - TypeError: Cannot read properties of null (reading 'nextInvocation')

Benchmark (only OK)

croner          x 86,291 ops/sec ±4.50% (89 runs sampled)
cronosjs        x 56,134 ops/sec ±4.17% (88 runs sampled)

Fastest is croner         

Pattern 1 2 3 4 5 6

Tests

cron            - OK    - 2024-05-04 03:02:01
croner          - OK    - 2024-05-04 03:02:01
cronosjs        - OK    - 2024-05-04 03:02:01
node-cron       - FAIL  - 1970-01-01 00:00:00
node-schedule   - OK    - 2024-05-04 03:02:01

Benchmark (only OK)

cron            x 6,313 ops/sec ±7.07% (84 runs sampled)
croner          x 160,651 ops/sec ±2.95% (87 runs sampled)
cronosjs        x 55,693 ops/sec ±4.69% (87 runs sampled)
node-schedule   x 2,726 ops/sec ±4.22% (85 runs sampled)

Fastest is croner         

Pattern */3 */3 */3 * * *

Tests

cron            - OK    - 2023-10-09 21:00:00
croner          - OK    - 2023-10-09 21:00:00
cronosjs        - OK    - 2023-10-09 21:00:00
node-cron       - FAIL  - 1970-01-01 00:00:00
node-schedule   - OK    - 2023-10-09 21:00:00

Benchmark (only OK)

cron            x 21,765 ops/sec ±5.38% (83 runs sampled)
croner          x 161,614 ops/sec ±3.57% (86 runs sampled)
cronosjs        x 32,285 ops/sec ±8.05% (88 runs sampled)
node-schedule   x 16,514 ops/sec ±4.31% (88 runs sampled)

Fastest is croner         

Pattern 0 0 0 29 2 1

Tests

cron            - OK    - 2024-02-05 00:00:00
croner          - OK    - 2024-02-05 00:00:00
cronosjs        - OK    - 2024-02-05 00:00:00
node-cron       - FAIL  - 1970-01-01 00:00:00
node-schedule   - OK    - 2024-02-05 00:00:00

Benchmark (only OK)

cron            x 9,744 ops/sec ±6.65% (84 runs sampled)
croner          x 161,572 ops/sec ±4.47% (88 runs sampled)
cronosjs        x 51,609 ops/sec ±5.92% (81 runs sampled)
node-schedule   x 5,341 ops/sec ±6.21% (82 runs sampled)

Fastest is croner         

Pattern 0 0 0 29 2 *

Tests

cron            - OK    - 2024-02-29 00:00:00
croner          - OK    - 2024-02-29 00:00:00
cronosjs        - OK    - 2024-02-29 00:00:00
node-cron       - FAIL  - 1970-01-01 00:00:00
node-schedule   - OK    - 2024-02-29 00:00:00

Benchmark (only OK)

cron            x 3,104 ops/sec ±3.65% (86 runs sampled)
croner          x 176,714 ops/sec ±4.69% (86 runs sampled)
cronosjs        x 67,920 ops/sec ±3.43% (89 runs sampled)
node-schedule   x 737 ops/sec ±3.69% (85 runs sampled)

Fastest is croner         

Pattern 15 15 */3 * * *

Tests

cron            - OK    - 2023-10-09 21:15:15
croner          - OK    - 2023-10-09 21:15:15
cronosjs        - OK    - 2023-10-09 21:15:15
node-cron       - FAIL  - 1970-01-01 00:00:00
node-schedule   - OK    - 2023-10-09 21:15:15

Benchmark (only OK)

cron            x 5,232 ops/sec ±3.66% (86 runs sampled)
croner          x 189,412 ops/sec ±4.44% (88 runs sampled)
cronosjs        x 40,316 ops/sec ±3.56% (89 runs sampled)
node-schedule   x 2,313 ops/sec ±4.69% (84 runs sampled)

Fastest is croner         

Pattern 15 15 */3 */10 10 *

Tests

cron            - OK    - 2023-10-11 00:15:15
croner          - OK    - 2023-10-11 00:15:15
cronosjs        - OK    - 2023-10-11 00:15:15
node-cron       - FAIL  - 1970-01-01 00:00:00
node-schedule   - OK    - 2023-10-11 00:15:15

Benchmark (only OK)

cron            x 5,129 ops/sec ±4.01% (86 runs sampled)
croner          x 158,104 ops/sec ±6.95% (81 runs sampled)
cronosjs        x 55,083 ops/sec ±3.85% (87 runs sampled)
node-schedule   x 2,622 ops/sec ±5.25% (80 runs sampled)

Fastest is croner         

Pattern 15 15 */3 * 10 SUN,MON,TUE

Tests

cron            - OK    - 2023-10-09 21:15:15
croner          - OK    - 2023-10-09 21:15:15
cronosjs        - OK    - 2023-10-09 21:15:15
node-cron       - FAIL  - 1970-01-01 00:00:00
node-schedule   - OK    - 2023-10-09 21:15:15

Benchmark (only OK)

cron            x 5,131 ops/sec ±4.00% (88 runs sampled)
croner          x 111,913 ops/sec ±3.74% (88 runs sampled)
cronosjs        x 42,010 ops/sec ±3.53% (88 runs sampled)
node-schedule   x 2,275 ops/sec ±6.14% (83 runs sampled)

Fastest is croner         

Test summary

Library OK FAIL % OK
cron 7 4 64%
croner 11 0 100%
cronosjs 10 1 91%
node-cron 0 11 0%
node-schedule 9 2 81%

@Hexagon
Copy link
Author

Hexagon commented Oct 9, 2023

PR updated to croner@7.0.2 and rebased onto nestjs:master.

@RDeluxe
Copy link

RDeluxe commented May 21, 2024

As the PR seems ready to go, we would love to see it merged as node-cron has several critical bugs.

Edit: I was targeting the wrong node-cron... sorry for the noise.

@sheerlox
Copy link
Contributor

As the PR seems ready to go, we would love to see it merged as node-cron has several critical bugs.

Hi, I would love to know what kind of critical bug you're encountering with cron. Please let us know in our issues!

As a note, the only bugs remaining (except the very recent DST one) occur extremely rarely, and we aren't able to replicate them despite our efforts and constant use of the library. Moreover, the users reporting the bug become silent when we ask them to provide debugging information or reproduction means. So if you're having one of these issues, please let us know!

@RDeluxe
Copy link

RDeluxe commented May 21, 2024

Hello @sheerlox!

I was checking the issues there : https://github.com/node-cron/node-cron/issues, and did not realize it was not the right repository...

We have been having memory issues since we started using nestjs/schedule, and found this issue. We thought we had found the culprit, but apparently it is not the case.

We will try to create a minimal reproduction repository (but it is commercial code, you know how it goes).

I'll remove my comment above.

@lukas-becker0
Copy link

How about supporting multiple backends ?
Then everybody would be happy I guess.

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.

5 participants