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

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

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

Conversation

Hexagon
Copy link

@Hexagon Hexagon commented Mar 3, 2023

  • Adjusts croner integration to croner 6.x
  • Pin npm install command in readme to 6.x
  • Fixes some tests that should not have succeded, but did on croner 5.x

Fixes #167

@Hexagon Hexagon force-pushed the bugfix/adjust-and-pin-croner-to-6.x branch from f9101bb to 56edbd5 Compare March 5, 2023 19:55
@Hexagon
Copy link
Author

Hexagon commented Mar 5, 2023

Updated for [email protected]

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

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

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

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

@kibertoad
Copy link
Owner

I haven't forgotten about it, and looked into it in several different occassions, but I haven't figured out what exactly goes wrong there. Will give it another short in the nearby future

@KillerJulian
Copy link

@kibertoad any update?

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.

Breaking changes in croner
3 participants