Skip to content

Stop the host first and then dispose it! #29404

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 20, 2021
Merged

Stop the host first and then dispose it! #29404

merged 3 commits into from
Jan 20, 2021

Conversation

ShreyasJejurkar
Copy link
Contributor

Fixes #29348
Fixes #25857

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 18, 2021
@davidfowl
Copy link
Member

Where's the test?

@ShreyasJejurkar
Added test
4a7c988 
Not sure if this is correct way to tests. As _host is private field in WebApplicationFactory<T>
@ShreyasJejurkar
Copy link
Contributor Author

@davidfowl I added the test, but not sure if this is the correct way to test. Because _host is a private field!

@davidfowl
Copy link
Member

What are you trying to fix?

@ShreyasJejurkar
Copy link
Contributor Author

I have linked corresponding issue in first link.

@davidfowl
Copy link
Member

I have linked corresponding issue in first link.

Then why isn't there a test that verifies the behavior? Are you clear on what event isn't firing and what event should be firing after the fix?

@ShreyasJejurkar
Copy link
Contributor Author

I did not found any existing tests that verifies that bug!
In my second commit, I added the new test, but right now am not sure that test is 100% correct.
Because I wanted to check _host is stopped or not before dispose, but the problem is that variable is private to that class and so cannot able to access it in test class!

javiercn
javiercn reviewed Jan 18, 2021
View reviewed changes
src/Mvc/Mvc.Testing/src/WebApplicationFactory.cs
}

_server?.Dispose();
_host?.StopAsync().Wait();
Copy link
Member

Choose a reason for hiding this comment

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

If we want to test that this is called, we could

  • Create the factory, request the IapplicationLifetime service from the host services (available through the factory).
  • Register a callback on stop.
  • Dispose the factory.
  • Validate the callback was invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very thanks @javiercn. I added the corresponding test as per your instructions, but instead of IApplicationLifetime, I used IHostApplicationLifetime as IApplicationLifetime is obsolete.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I didn't remember exactly how it was named, so perfect!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😀

@mkArtakMSFT mkArtakMSFT added the community-contribution Indicates that the PR has been added by a community member label Jan 19, 2021
javiercn
javiercn approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Great set of changes

@javiercn javiercn merged commit 69e040e into dotnet:master Jan 20, 2021
@ShreyasJejurkar
Copy link
Contributor Author

Hey @javiercn, I forget to remove the leftover bool variable!! Should I create a new PR, or is there any way to get this done in this PR, as you just merged it!

@ghost
Copy link

ghost commented Jan 20, 2021

Hi @MCCshreyas. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@javiercn
Copy link
Member

@MCCshreyas If you want to send a separate PR to clean it up that's fine with us. I was about to comment on this PR to point out that we should also implement IAsyncDisposable since I'm not a fan of the .Wait() there.

I've filed a follow up issue here if you feel like tackling it

@ghost
Copy link

ghost commented Jan 20, 2021

Hi @javiercn. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@javiercn javiercn mentioned this pull request Jan 20, 2021
Closed
@ShreyasJejurkar
Copy link
Contributor Author

@javiercn Ok. I will check that out, and will open PR!

@ghost
Copy link

ghost commented Jan 20, 2021

Hi @MCCshreyas. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@ShreyasJejurkar ShreyasJejurkar mentioned this pull request Jan 20, 2021
Closed
@quixoticaxis
Copy link

@MCCshreyas it does not look like PR fixes the #25857. IHostApplicationLifetime.StopApplication is still ignored.

The PR only fixes my issue #29348. Big thanks!

@ghost
Copy link

ghost commented Jan 20, 2021

Hi @quixoticaxis. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@ShreyasJejurkar ShreyasJejurkar deleted the stop-host branch April 13, 2021 06:27
HEskandari pushed a commit to HEskandari/aspnetcore that referenced this pull request May 13, 2021
@ShreyasJejurkar @HEskandari
Implemented IAsyncDisposable for WebApplicationFactory
3d6f75a 
1. Other fix - Removed unwanted variable was added as part of dotnet#29404

Fixes dotnet#29455
HEskandari pushed a commit to HEskandari/aspnetcore that referenced this pull request May 17, 2021
@ShreyasJejurkar @HEskandari
Implemented IAsyncDisposable for WebApplicationFactory
40d5510 
1. Other fix - Removed unwanted variable was added as part of dotnet#29404

Fixes dotnet#29455
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiercn javiercn javiercn approved these changes

@pranavkm pranavkm Awaiting requested review from pranavkm

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IHostApplicationLifetime StopApplication not respected in AspNetCore.Mvc.Testing Let AspNetCore.Mvc.Testing stop the host before disposing it
5 participants
@ShreyasJejurkar @davidfowl @javiercn @quixoticaxis @mkArtakMSFT