Exceptions in jobs are swallowed

[skarum]

Hi,

I have been testing this and I like a lot of what I see, but have just spent an embarrassing amount of time figuring out why my job never got invoked even though the log said it was.

As far as I can see the job executor swallows all exceptions and nothing bubbles out to the log. https://github.com/linkdotnet/NCronJob/blob/f8eacb2affb483d1e0bb91cd70f32f49b6de4309/src/LinkDotNet.NCronJob/Execution/JobExecutor.cs#L41

Could this be changed or made configurable? I'm happy to make a PR if you are interested.

[linkdotnet]

Hey @skarum,

currently, this is "by design". The current approach has the issue, that, if an exception is thrown in the continuation, we can't catch it. That is why we introduced the INotificationHandler<TJob> that passes in the Exception? exc if there was one.

The interesting bit for me is why the job was never invoked in the first place. The referenced line/function isn't doing much besides really kickstarting the job.

Could this be changed

Yes, absolutely - there is an open PR (#21 by @falvarez1) after which we can more easily implement such logic (or it is tackle already to some extent)

[skarum]

I think I know why it initially did not get trigged.

when the job gets resolved from the container ServiceProvider.GetService is called and even though it returns null the type is still of IJob.

If https://github.com/linkdotnet/NCronJob/blob/9b3fe21cd3549923d1d4802f65546849d827e60a/src/LinkDotNet.NCronJob/Execution/JobExecutor.cs#L30 was changed to if (scope.ServiceProvider.GetRequiredService(run.Type) is not IJob job) it should throw.

[linkdotnet]

Hmmm, yes that wasn't the best decision from me, just to log this information out instead of throwing an exception.

The GetService was on purpose (instead of GetRequiredService which instantly throws), so that, even if one job is not properly registered, it does not interfere with other runs.

In retrospective I do understand that is difficult to find out. Would you prefer a "real" exception here rather than a simple log-statement?

[skarum]

I would prefer a real exception as this is not a transient error, but the result of my app being setup wrong.

[linkdotnet]

Fair enough - if you want, you can file a PR with the suggested change (GetRequiredService instead of checking and logging).

It should not interfere with the ongoing PR (#21).

I would prefer a real exception as this is not a transient error, but the result of my app being setup wrong.

That is true - the "problem" is that this gets "only" checked once a job is executed. So you might run your application for X hours/days until this exception comes up.

EDIT: Flagged as "enhancement" because it was a deliberate choice of mine to have it that way (and not a bug :D)

[skarum]

I'll make a PR :-)

[linkdotnet]

Thanks - much appreciated!