Skip to content
This repository has been archived by the owner on Sep 10, 2024. It is now read-only.

fix: remove invalid exn handling #132

Merged

Conversation

stijnmoreels
Copy link
Member

Remove the attempt to log on unobserved exceptions.
We tried to retrieve back the registered logging system, but this is not only wrong, it is rather unstable.

We can either remove this, like in this PR, so that the consumers can see directly what's wrong with their scheduled job (authentication, processing failures...)
Or we can come up with a more resilient scheduled job that logs critical errors for unhandled exceptions in the scheduled job instead on relying on the incomplete 'unobsered exception handling'.

See also kdcllc/CronScheduler.AspNetCore#47 and kdcllc/CronScheduler.AspNetCore#48

@stijnmoreels stijnmoreels requested a review from fgheysels as a code owner May 2, 2022 08:14
@stijnmoreels stijnmoreels added this to the v0.4.0 milestone May 2, 2022
@netlify
Copy link

netlify bot commented May 2, 2022

Deploy Preview for arcus-background-jobs canceled.

Name Link
🔨 Latest commit 29bd10e
🔍 Latest deploy log https://app.netlify.com/sites/arcus-background-jobs/deploys/62723fbf099c5f0008a79b58

@fgheysels
Copy link
Member

So, what exactly happens now with removing this code ? The background-job crashes on unhandled exceptions ?

@stijnmoreels
Copy link
Member Author

So, what exactly happens now with removing this code ? The background-job crashes on unhandled exceptions ?

Well, internally, the CronScheduler uses .NET hosted services, so the service will throw an exception, that's correct. The problem, really, is that we could have an handling logic in our background job, but we do not have access to the service provider or the dependency system. I created an issue here kdcllc/CronScheduler.AspNetCore#47 and provided a solution here kdcllc/CronScheduler.AspNetCore#48 but until now, there's no one responding 😢 .

@stijnmoreels stijnmoreels removed their assignment May 4, 2022
@fgheysels
Copy link
Member

So, what exactly happens now with removing this code ? The background-job crashes on unhandled exceptions ?

Well, internally, the CronScheduler uses .NET hosted services, so the service will throw an exception, that's correct. The problem, really, is that we could have an handling logic in our background job, but we do not have access to the service provider or the dependency system. I created an issue here kdcllc/CronScheduler.AspNetCore#47 and provided a solution here kdcllc/CronScheduler.AspNetCore#48 but until now, there's no one responding 😢 .

Looks like the repo isn't getting that much of attention/commits anyway ...
When the hosted service throws an exception that is not handled, isn't that service going to stop exectuing without further notice ?

@stijnmoreels
Copy link
Member Author

So, what exactly happens now with removing this code ? The background-job crashes on unhandled exceptions ?

Well, internally, the CronScheduler uses .NET hosted services, so the service will throw an exception, that's correct. The problem, really, is that we could have an handling logic in our background job, but we do not have access to the service provider or the dependency system. I created an issue here kdcllc/CronScheduler.AspNetCore#47 and provided a solution here kdcllc/CronScheduler.AspNetCore#48 but until now, there's no one responding 😢 .

Looks like the repo isn't getting that much of attention/commits anyway ... When the hosted service throws an exception that is not handled, isn't that service going to stop exectuing without further notice ?

Yeah, and I was just looking at their code, seems that in that case the exception is also not logged. Hmm, maybe we should implement our own exception handling strategy and let all our background jobs inherit from that.

@stijnmoreels stijnmoreels removed their assignment May 4, 2022
@stijnmoreels
Copy link
Member Author

Added some exception handling in the jobs themselves.

@fgheysels
Copy link
Member

handling strategy and let all our backg

That might be a good idea.

@stijnmoreels
Copy link
Member Author

handling strategy and let all our backg

That might be a good idea.

Already done this in the changes, feel free to take a look. 👍

@stijnmoreels stijnmoreels removed their assignment May 5, 2022
Copy link
Member

@fgheysels fgheysels left a comment

Choose a reason for hiding this comment

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

Great.

@fgheysels fgheysels merged commit 84f75db into arcus-azure:master May 9, 2022
@kdcllc
Copy link

kdcllc commented Jun 12, 2022

So, what exactly happens now with removing this code ? The background-job crashes on unhandled exceptions ?

Well, internally, the CronScheduler uses .NET hosted services, so the service will throw an exception, that's correct. The problem, really, is that we could have an handling logic in our background job, but we do not have access to the service provider or the dependency system. I created an issue here kdcllc/CronScheduler.AspNetCore#47 and provided a solution here kdcllc/CronScheduler.AspNetCore#48 but until now, there's no one responding 😢 .

I am sorry for the delay in including your functionality. I released the latest code todayhttps://github.com/kdcllc/CronScheduler.AspNetCore/pull/50. I hope this helps

@stijnmoreels stijnmoreels deleted the fix/remove-invalid-exn-handling branch June 13, 2022 05:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract UnobservedExceptionHandler for scheduled background jobs into a Arcus.BackgroundJobs.Core project
3 participants