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

Scheduled func will return false when a DoSafely function is used #131

Open
prassag opened this issue Dec 2, 2019 · 4 comments
Open

Scheduled func will return false when a DoSafely function is used #131

prassag opened this issue Dec 2, 2019 · 4 comments

Comments

@prassag
Copy link

prassag commented Dec 2, 2019

The DoSafely function hides the function name (jobFun) so the function written to the job is always the wrapper recoveryWrapperFunc.

@Streppel
Copy link
Collaborator

Streppel commented Dec 6, 2019

Thank you for taking the time to let us know @prassag! I believe we might consider stripping out the DoSafely function altogether in the near future and stop bubbling panics from inside the code instead. Correct error handling is coming up in a new branch that will be merged as soon as we finish conflicts

@preslavrachev
Copy link
Contributor

Sad to see the DoSafely function causing so many issues. I might try to revise it an come up with a quick fix over the weekend.

@Streppel I am not sure that preventing panics in 100% of the cases (if I understood you correctly) is the safest solution. There may be many situations, in which a run-time panic may cause the application to continue in a broken state. In such situations, a clean app restart works best, or some sort of state cleanup is needed. The idea behind DoSafely was that the caller would consciously use it over Do and accept such possible risks.

That's why, even when wrapping the panic, IMHO, a custom error needs to be propagated back to the caller. Whoever sets up the scheduled jobs needs to have some possibility of writing custom error handling code.

@Streppel
Copy link
Collaborator

Streppel commented Jan 3, 2020

@preslavrachev absolutely Preslav! I'm not saying we will stop preventing panics, what I meant is that we will stop propagating panics deliberately from within the code, so we can keep the panic detection and handling inside the bare Do function instead of another one (say DoSafely). I mean, that's the main reason why DoSafely was implemented as another function, right? As there were panics being called directly on the code in case of simple errors.

I believe this is our chance to review how this feature could be better implemented. There will be a merge in a few days which will deprecate DoSafely and thus we can focus on working directly on Do then :) what do you say?

@JohnRoesler
Copy link
Contributor

@preslavrachev and others, come checkout the fork we're maintaining https://github.com/go-co-op/gocron

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants