Reduce reliance on DI container

nulltoken commented 4 weeks ago

Pull request description

This is a very first attempt at untangling registration from consumption of services, in order to later be able to maybe surface the IServiceProvider to the NCronJobBuilder (cf. . #37)

This approach reduces the reliance of NCronJob to the DI container to the bare minimum: JobDefinitions and friends are no longer stored there. Instead, a lightweight container holds all of them. The rationale being that IServiceProvider is a read-only container, whereas JobDefinitions are more "dynamic". They can be updated, removed, .... As such, storing them in an alternate container seems to go in the right direction and may lead to some simpler code in the end.

This first step also puts some further potential opportunities for change under the light. For instance, the boundaries between DontKnowHowToNameThat storage and JobRegistry become very blurry.

@linkdotnet This whole thing has been hacked through. It's of course not a definitive PR, rather a way to share thoughts and see if this approach could lead somewhere that may make sense to you.

Thoughts?

PR meta checklist

[X] Pull request is targeted at main branch for code
[ ] Pull request is linked to all related issues, if any.

Code PR specific checklist

[ ] My code follows the code style of this project and AspNetCore coding guidelines.
[ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
[ ] I have updated the appropriate sub section in the CHANGELOG.md.
[X] I have added, updated or removed tests to according to my changes.
- [x] All tests passed.

nulltoken commented 4 weeks ago

@linkdotnet I seem to be suffering from a case of flaky test. ShouldExecuteRuntimeJobsEvenWhenCurrentJobIsFarInTheFuture seem to randomly pass/fail (independently of the underlying OS).

Would you have any insight?

linkdotnet commented 4 weeks ago

Hey @nulltoken

Thanks for the initiative! Much appreciated. I try to have a look tonight if time allows it.

Regarding the test - unfortunately that is an oversight on my site. It never fails locally, but it seems to have somewhere a race condition or otherwise a problem. For now, you can safely ignore it. It is on my radar.

linkdotnet commented 4 weeks ago

By the way, I like your rationale. The. RuntimeRegistry does similar things, so maybe there is a chance to merge everything into one big "thing" just called registry?

nulltoken commented 4 weeks ago

By the way, I like your rationale. The. RuntimeRegistry does similar things, so maybe there is a chance to merge everything into one big "thing" just called registry?

Yep. The "blurry boundaries" I was mentioning seem to point toward this kind of approach. And getting this one big "thing" might also make dealing with dependent jobs a bit easier.

There's still something that I need to better understand regarding the JobDefinition lifecycle: The JobRegistry lock them in a deduplicated way through a hashset, whereas ExecuteWhenHelper.AddRegistration() needs them unfolded in order to able to access the last registered JobDefinition.

nulltoken commented 4 weeks ago

so maybe there is a chance to merge everything into one big "thing" just called registry?

I pushed some minor changes in that direction. The JobRegistry has now temporarily turned into a big ball of mud :wink:

nulltoken commented 4 weeks ago

@linkdotnet Ok. This might be ready for a first round of review.

Main objective of the proposal is to no longer use the DI container to store JobDefinitions and friends, and rather leverage JobRegistry for this intent.
In order to more easily keep everything in sync, DependingJobRegistry and DynamicJobFactoryRegistry have been melded into JobRegistry.
Although a lot of files have been touched, I've done my best to reduce the diff noise as much as possible.

Note: JobRegistry may need some further rewrite as it's a bit "puffy" now.

linkdotnet commented 3 weeks ago

Sorry for the late response - I try tomorrow morning as I was busy today

nulltoken commented 3 weeks ago

Note: JobRegistry may need some further rewrite as it's a bit "puffy" now.

@linkdotnet I've pushed as much logic as I could think of downward into the JobRegistry. Are there any other low hanging related refactorings that you'd like to see done in the context of this PR?

nulltoken commented 3 weeks ago

FWIW, I've just pushed a minor refactoring in JobRegistry.AddDynamicJob()

linkdotnet commented 3 weeks ago

That flaky test is the first thing after the merge I'll kick out. Sorry for that

linkdotnet commented 3 weeks ago

If there isn't anything open, I'd love to merge the changes

falvarez1 commented 3 weeks ago

The rationale being that IServiceProvider is a read-only container, whereas JobDefinitions are more "dynamic". They can be updated, removed, .... As such, storing them in an alternate container seems to go in the right direction and may lead to some simpler code in the end.

I like this line of thinking. Code looks looks good so far. I'll test this out thoroughly this evening.

falvarez1 commented 3 weeks ago

If there isn't anything open, I'd love to merge the changes

@linkdotnet I'm good with this 👍

linkdotnet commented 3 weeks ago

Forgot to say: @nulltoken - you might want to add an entry to the CHANGELOG.MD with your credits. I will do that.

NCronJob-Dev / NCronJob