Skip to content

Adjust croner integration to 6.x. Add version in README.md. #168

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.

Sign up for GitHub

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

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

Adjust croner integration to 6.x. Add version in README.md. #168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ You can use CronJob instances for handling Cron-style scheduling:
scheduler.addCronJob(job)
```

Note that you need to install "croner" library for this to work. Run `npm i croner` in order to install this dependency.
Note that you need to install "croner" library for this to work. Run `npm i croner@6` in order to install this dependency.

## Usage in clustered environments

Expand Down
11 changes: 4 additions & 7 deletions lib/engines/cron/CronJob.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export type CronSchedule = {
}

export type Cron = {
running(): boolean
isRunning(): boolean
stop(): void
}

Expand All @@ -39,7 +39,7 @@ export class CronJob extends Job {

/* istanbul ignore next */
getStatus(): JobStatus {
return this.cronInstance?.running() ? JobStatus.RUNNING : JobStatus.STOPPED
return this.cronInstance?.isRunning() ? JobStatus.RUNNING : JobStatus.STOPPED
}

start(): void {
Expand All @@ -56,12 +56,9 @@ export class CronJob extends Job {
this.schedule.cronExpression,
{
timezone: this.schedule.timezone,
protect: this.preventOverrun
},
() => {
if (!this.task.isExecuting || !this.preventOverrun) {
this.task.execute()
}
}
async () => await this.task.execute()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hexagon Is it necessary to have an async function here? Wouldn't it suffice to do return Promise.resolve().then(() => this.task.execute())?

Copy link
Author

@Hexagon Hexagon Mar 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kibertoad Main reason for the async/await is that croner awaits the call internally, to make .isBusy() of the returned job work. Not too familiar with "raw" promises, guess you know better than me what should be used here 👀

() => return this.task.execute() might pass the promise further, allowing the croner await to work?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you are awaiting it anyway, doing so here makes little sense.
mixing async functions and callbacks can be error-prone, so I would avoid introducing async function without a good reason.
() => return this.task.execute() might pass the promise further, allowing the croner await to work?
if croner does not necessarily expect a fully formed promise there (e. g. does not attach .then or .catch handlers to it), that should be sufficient, as await will work with both synchronous and asynchronous tasks, yes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Will give that a try

Copy link
Author

@Hexagon Hexagon Mar 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the overrun-test (allows preventing CronJob execution overrun with async tasks) only pass using async/await

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what error are you getting?

Copy link
Author

@Hexagon Hexagon Mar 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overrun protection does not work, the function is triggered again after 4000ms even though it should be blocking for 5000ms (pattern */2 * * ..)

This is how croner is expected to handle it https://github.com/Hexagon/croner/blob/master/docs/EXAMPLES.md#overrun-protection

... and the overrun protection works by setting blocked = x before and after calling the function, and awaiting the function

https://github.com/Hexagon/croner/blob/9a66976992a571074c9f508e02f9746649dd5a67/src/croner.js#L421

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you are depending on it being a function then.
So I would suggest going the Promise.resolve route then

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't seem to get this test working, can you have a look?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I'll check it out tomorrow

)
}

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"@types/node": "^18.11.17",
"@typescript-eslint/eslint-plugin": "^5.47.0",
"@typescript-eslint/parser": "^5.47.0",
"croner": "^5.3.5",
"croner": "^6.0.2",
"eslint": "^8.30.0",
"eslint-config-prettier": "^8.5.0",
"eslint-plugin-prettier": "^4.2.1",
Expand Down
33 changes: 21 additions & 12 deletions test/CronJob.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe('ToadScheduler', () => {
expect(job.getStatus()).toBe('stopped')
})

it('correctly processes CronJob', () => {
it('correctly processes CronJob', async () => {
let counter = 0
const scheduler = new ToadScheduler()
const task = new Task('simple task', () => {
Expand All @@ -63,12 +63,16 @@ describe('ToadScheduler', () => {
resetTime()
expect(counter).toBe(0)
advanceTimersByTime(19999)
await Promise.resolve(undefined)
expect(counter).toBe(0)
advanceTimersByTime(1)
await Promise.resolve(undefined)
expect(counter).toBe(1)
advanceTimersByTime(19999)
await Promise.resolve(undefined)
expect(counter).toBe(1)
advanceTimersByTime(1)
await Promise.resolve(undefined)
expect(counter).toBe(2)

scheduler.stop()
Expand Down Expand Up @@ -126,7 +130,7 @@ describe('ToadScheduler', () => {
it('allows enabling CronJob execution overrun', async () => {
let counter = 0
const scheduler = new ToadScheduler()
const task = new AsyncTask('simple task', () => {
const task = new AsyncTask('simple task', async () => {
counter++
advanceTimersByTime(5000)
return Promise.resolve(undefined)
Expand All @@ -152,13 +156,13 @@ describe('ToadScheduler', () => {
await Promise.resolve() // this allows promises to play nice with mocked timers
await Promise.resolve()
await Promise.resolve()
expect(counter).toBe(4)
expect(counter).toBe(3)
advanceTimersByTime(1000)
advanceTimersByTime(1000)
advanceTimersByTime(1000)
advanceTimersByTime(1000)
advanceTimersByTime(1000)
expect(counter).toBe(9)
expect(counter).toBe(8)
await Promise.resolve()
await Promise.resolve()
await Promise.resolve()
Expand All @@ -168,14 +172,14 @@ describe('ToadScheduler', () => {
advanceTimersByTime(1000)
advanceTimersByTime(1000)
await Promise.resolve()
expect(counter).toBe(14)
expect(counter).toBe(13)
scheduler.stop()
})

it('allows preventing CronJob execution overrun with async tasks', async () => {
let counter = 0
const scheduler = new ToadScheduler()
const task = new AsyncTask('simple task', () => {
const task = new AsyncTask('simple task', async () => {
counter++
advanceTimersByTime(5000)
return Promise.resolve(undefined)
Expand All @@ -199,10 +203,11 @@ describe('ToadScheduler', () => {
advanceTimersByTime(1)
await Promise.resolve()
expect(counter).toBe(1)
advanceTimersByTime(2000)
await Promise.resolve()
advanceTimersByTime(999)
expect(counter).toBe(1)
advanceTimersByTime(1)
advanceTimersByTime(6000)
await Promise.resolve()
expect(counter).toBe(2)
scheduler.stop()
})
Expand All @@ -214,7 +219,7 @@ describe('ToadScheduler', () => {
let result3 = 0

const scheduler = new ToadScheduler()
const task = new AsyncTask('simple task', () => {
const task = new AsyncTask('simple task', async () => {
counter++
advanceTimersByTime(5000)
const promise1 = Promise.resolve().then(() => {
Expand Down Expand Up @@ -255,11 +260,11 @@ describe('ToadScheduler', () => {
expect(result3).toBe(1)
expect(counter).toBe(1)
await Promise.resolve()
advanceTimersByTime(999)
advanceTimersByTime(1999)
expect(counter).toBe(1)
await Promise.resolve()
await Promise.resolve()
advanceTimersByTime(1)
advanceTimersByTime(2000)
await Promise.resolve()
expect(counter).toBe(2)
scheduler.stop()
Expand All @@ -268,7 +273,7 @@ describe('ToadScheduler', () => {
expect(result3).toBe(2)
})

it('correctly handles adding job twice', () => {
it('correctly handles adding job twice', async () => {
let counter = 0
const scheduler = new ToadScheduler()
const task = new Task('simple task', () => {
Expand All @@ -286,12 +291,16 @@ describe('ToadScheduler', () => {
resetTime()
expect(counter).toBe(0)
advanceTimersByTime(19999)
await Promise.resolve()
expect(counter).toBe(0)
advanceTimersByTime(1)
await Promise.resolve()
expect(counter).toBe(2)
advanceTimersByTime(19999)
await Promise.resolve()
expect(counter).toBe(2)
advanceTimersByTime(1)
await Promise.resolve()
expect(counter).toBe(4)

scheduler.stop()
Expand Down