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

Do not run IHostedService implementations #2713

Conversation

desjoerd
Copy link

Currently the CLI is running all registered Hosted Services (IHostedService). I see that it's needed to start the application because otherwise the app.Use*/app.Map* configuration is not run. Because it's starting the application (with a NoopServer) it is running all registered HostedServices.

Because the CLI is almost always run in the context of a build it would be nice to not run those registered hosted services. This would also solve instances of for example HangFire or other things which start an IHostedService.

Please let me know if you want to have this change or if I need to modify something.

Related issue: #2712

Copy link

@siriak siriak left a comment

Choose a reason for hiding this comment

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

I had a similar situation where we have validation on config values and when during build in CI environment we would generate swagger.json, it'd crash because of missing config files. So, this PR wouldn't solve my problem, but if it works for other people in their cases, I still think it's worthy!

@desjoerd
Copy link
Author

Thanks for the review, I see the build is currently (still) failing which is blocking the merge. I think #2730 would solve that.

Havunen added a commit to Havunen/DotSwashbuckle2 that referenced this pull request Feb 12, 2024
Havunen added a commit to Havunen/DotSwashbuckle that referenced this pull request Feb 18, 2024
@martincostello
Copy link
Collaborator

Thanks for contributing - if you'd like to continue with this pull request, please rebase against the default branch to pick up our new CI.

@desjoerd
Copy link
Author

Thanks for contributing - if you'd like to continue with this pull request, please rebase against the default branch to pick up our new CI.

Does this mean you're going to pick up active development?

@martincostello
Copy link
Collaborator

See #2778 🙂

@desjoerd
Copy link
Author

I will update my PR, also the implementation was a bit too eager (which I fixed in Nswag) so I will put that in as well. Probably somewhere in the coming week.

@martincostello
Copy link
Collaborator

I've pulled this changes into #2880.

@desjoerd if you could clarify what the missing fix to stop it being "too eager" is, I can apply that change and then move the new PR out of draft.

@desjoerd
Copy link
Author

Awesome, I totally got swamped in work. I've added a review :).

@martincostello martincostello added this to the v6.6.2 milestone May 14, 2024
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.

None yet

3 participants