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

Exclude IHostedService implementations when running Console Commands #4523

Merged

Conversation

desjoerd
Copy link
Contributor

Exclude all IHostedService implementations except GenericWebHostService because it Configures the pipeline.

By removing the IHostedService implementations it will stop running these when starting the app which happens with the WebApplication builder.

Currently the CLI is running all registered Hosted Services (IHostedService). 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 Console Commands are 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.

…ce because it Configures the pipeline.

By removing the IHostedService implementations it will stop running these when starting the app which happens with the WebApplication builder.
@desjoerd
Copy link
Contributor Author

desjoerd commented Sep 26, 2023

This PR is coming out of the blue, please let me know if I need to add unit tests or an example which shows the current/fixed behavior. I see that there is no unit test project yet for the ConsoleCore or Commands project.

If a github issue is wanted I am happy to create this :).

It would fix #3922 by allowing the normal flow to work with IHostedService implementations

@RicoSuter
Copy link
Owner

Arent there any cases where these are actually needed? I cannot think of one...

@desjoerd
Copy link
Contributor Author

desjoerd commented Sep 26, 2023

Me neither, except for the GenericWebHostService. It's unfortunate that that one is building request pipeline, so something to keep an eye on. I've got a similar PR on Swashbuckle (lovely boilerplate code which is unfortunately not published by the dotnet team) -> domaindrivendev/Swashbuckle.AspNetCore#2713

Maybe it's good to add some unit tests for the Nswag Commands/ConsoleCore project. I was a bit hesitant because I don't know the wanted target framework, in Nswag.Core.Tests netcoreapp3.1 is used, but that one is out of support. Also it requires a project with a crashing (or traceable) hosted service.

So for me to add the unit tests I would like to know

  • The desired target framework (I don't fully know whether netcoreapp3.1 would work, it's not the runtime but still feels a bit weird).
  • The location for adding the Project which contains a faulty/traceable hosted service.

It can also go without unit tests, being pretty confident it doesn't break anything.

PS sorry in advanxe for typos or similar typing issues, on my phone ;)

@RicoSuter
Copy link
Owner

Let's first merge the v14 PR #3758 and then I want to add some console/command/cli unit tests because I removed most of the legacy manual tests for that (as they werent reliable etc).

I think I can write a unit test which runs consolecore against the .net 7 sample project and check whether the openapi.json is generated...

@RicoSuter
Copy link
Owner

Added some simple tests here:
https://github.com/RicoSuter/NSwag/blob/master/src/NSwag.ConsoleCore.Tests/GenerateSampleSpecificationTests.cs

Do you want to add more or should we merge the PR?

@desjoerd
Copy link
Contributor Author

desjoerd commented Sep 27, 2023

Looks awesome, this proofs that the happy flow is and will not be broken. I think this PR can be merged.

On a later stage I can maybe add a "weird" scenarios project to test more setup scenarios (not as a sample because it would confuse people).

@RicoSuter RicoSuter merged commit 5d4167d into RicoSuter:master Sep 28, 2023
1 check passed
lahma pushed a commit to lahma/NSwag that referenced this pull request Jan 20, 2024
…ce because it Configures the pipeline. (RicoSuter#4523)

By removing the IHostedService implementations it will stop running these when starting the app which happens with the WebApplication builder.
@RicoSuter
Copy link
Owner

It seems that there are cases where this is a problem:
#4722

@desjoerd
Copy link
Contributor Author

desjoerd commented Feb 2, 2024

That's unfortunate. I think we need to find another way to prevent the hosted services from running, but still keep them registered. I will have a look into that on Monday.

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

Successfully merging this pull request may close these issues.

None yet

2 participants