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

seal DefaultHttpContext and remove obsolete members #6504

Merged
merged 1 commit into from Jan 9, 2019

Conversation

@davidfowl
Copy link
Member

commented Jan 9, 2019

Spoke to @DamianEdwards offline about this breaking change so putting it here in a PR to review. I'm sure I need to update other things as well but this is the baseline. Motivation for this change comes from this PR #6424

PS: There are a class of other changes related to the DefaultHttpContext that we want to optimize:

aspnet/HttpAbstractions#999

cc @benaadams

@davidfowl davidfowl requested review from halter73, Tratcher and HaoK Jan 9, 2019
@JamesNK

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

Question from someone not super familiar with the internals of Http:

What is the reason for choosing to make DefaultHttpContext sealed compared to a new class that is a implementation of DefaultHttpContext?

public sealed class ReusableHttpContext : DefaultHttpContext { }

There would be overhead ensuring changes to DefaultHttpContext are then made reusable in ReusableHttpContext, but it would avoid the breaking change. Sealing could break a lot of test code based around mocking, e.g. Mock<DefaultHttpContext>

@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

What is the reason for choosing to make DefaultHttpContext sealed compared to a new class that is a implementation of DefaultHttpContext?

More overhead, more fields, more virtual calls. The fields inside of DefaultHttpContext are typed as the base type instead of the derived type because of the "flexibility".

There would be overhead ensuring changes to DefaultHttpContext are then made reusable in ReusableHttpContext, but it would avoid the breaking change. Sealing could break a lot of test code based around mocking, e.g. Mock

Yes, that's the biggest risk in this change. Test code trying to override members of DefaultHttpContext without using features.

@halter73

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

So you're planning on deleting ReusableHttpContext and going back to using DefaultHttpContext?

@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

@halter73

So you're planning on deleting ReusableHttpContext and going back to using DefaultHttpContext?

Right, if this gets approval. That's the idea.

@JamesNK I could unseal the DefaultHttpContext to make a bit less breaking so that Mock<DefaultHttpContext> would still work but I'm not there yet.

@JamesNK

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

Yes, that's the biggest risk in this change. Test code trying to override members of DefaultHttpContext without using features.

Ok! A mitigation could be to have something like TestableHttpContext in an official testing package. It would provided default virtual implementations without being sealed. Developers would then replace Mock<DefaultHttpContext> in tests with TestableHttpContext upon updating to 3.0.

@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

Ok! A mitigation could be to have something like TestableHttpContext in an official testing package. It would provided default virtual implementations without being sealed. Developers would then replace Mock<DefaultHttpContext> in tests with TestableHttpContext upon updating to 3.0.

Yes, that's a decent mitigation as well.

@halter73

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

Ok! A mitigation could be to have something like TestableHttpContext in an official testing package. It would provided default virtual implementations without being sealed. Developers would then replace Mock<DefaultHttpContext> in tests with TestableHttpContext upon updating to 3.0.

If you're using Moq anyway, what's wrong with Mock<HttpContext>?

It's also a little unfortunate that DefaultHttpContext.(Un)Initialize() is still public now that it's no longer virtual. Is there any reason a developer should ever call these instead of letting the server handle the pooling?

@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

It's also a little unfortunate that DefaultHttpContext.(Un)Initialize() is still public now that it's no longer virtual. Is there any reason a developer should ever call these instead of letting the server handle the pooling?

It should never be called by the end user but they are given an HttpContext anyways, they would need to downcast to even see these methods.

@JamesNK

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

@halter73 Mock<HttpContext> would be far more common.

The difference is HttpContext requires you to mock out all the behavior that you need, while DefaultHttpContext lets you take the defaults and override only what you care about.

Some results for mocking the default context, but less than I would have guessed:

https://github.com/search?q=%22Mock%3CDefaultHttpContext%3E%22&type=Code - 10 results
https://github.com/search?q=%22Mock%3CHttpContext%3E%22&type=Code - 7000 results

@davidfowl davidfowl force-pushed the davidfowl/faster-httpcontext branch from 3bdf03b to f08e8c9 Jan 9, 2019
@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

:trollface:

@benaadams

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Make the break 😍

Is a DefaultHttpContext not a CustomHttpContext 😉

Alternative would be to make the changes to DefaultHttpContext (minus init/uninit); then inhert for a PooledHttpContext and seal that adding the init/uninit methods; just to avoid copypasta for Features if creating your own HttpContext

@rynowak

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

The difference is HttpContext requires you to mock out all the behavior that you need, while DefaultHttpContext lets you take the defaults and override only what you care about.

In N years of building MVC core, I haven't happened upon a situation where mocking the context is a better choice than newing up a DefaultHttpContext. Do we know of cases like this?

@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

In N years of building MVC core, I haven't happened upon a situation where mocking the context is a better choice than newing up a DefaultHttpContext. Do we know of cases like this?

Most of the tests I see, new up the DefaultHttpContext (it was actually intentional to make it POCO like and newable). A few purists mock the HttpContext and 10 people on github mock the DefaultHttpContext. I'm sure even less have a derived type outside of that last scenario besides @benaadams. If it's too big of a break, we can unseal the DefaultHttpContext but we'd likely be missing out on some JIT optimizations (though TBH I'm not 100% sure if there are other things preventing that from happening).

@davidfowl davidfowl force-pushed the davidfowl/faster-httpcontext branch from 8dfee5e to 26164d0 Jan 9, 2019
- Seal all of the classes
- Remove virtual methods
- Delete pooled HttpContext code
- Removed obsolete AuthenticationManager
@davidfowl davidfowl force-pushed the davidfowl/faster-httpcontext branch from 26164d0 to 744be8c Jan 9, 2019
@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

So I'm guessing @DamianEdwards is going to have to sign off on this one, or @Eilon

Copy link
Member

left a comment

Breaking change approved at this stage given the data and discussed potential mitigations we could employ later on.

@davidfowl davidfowl merged commit a11c68a into master Jan 9, 2019
2 of 3 checks passed
2 of 3 checks passed
AspNetCore-ci Build #20190109.11 has test failures
Details
AspNetCore-pr-validation-temp Build #20190109.11 succeeded
Details
license/cla All CLA requirements met.
Details
@Eilon

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

Is this announcement-worthy?

@Tratcher

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

@HaoK this overlaps with #4485

@davidfowl

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

@HaoK

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

I can rebase my PR after

@benaadams

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

I'm sure even less have a derived type outside of that last scenario besides @benaadams.

I won't miss it; only do it for pooling, but its way over designed for that.

If it becomes both faster and pooled as part of the baseline, my use case (and I imagine anyone else's) has gone and is served by the base framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.