search

weidazhao / Hosting

Hosting prototype
170 stars 35 forks source link

Remove HttpContextAccessor use #8

Closed benaadams closed 8 years ago

benaadams commented 8 years ago

HttpContextAccessor is a form of evil...

weidazhao commented 8 years ago

Yep, this is much better than using HttpContextAccessor. Thanks Ben for the change.

Since the code in this repo will likely get moved to Service Fabric SDK eventually, I will need to go through the open source process internally before I can take your pull request. Let me get back to you once I figure it out.

weidazhao commented 8 years ago

Hi Ben, have you run Hosting.Tests with your change? They failed with your change on my box. @benaadams

benaadams commented 8 years ago

I'll have to check it again; I think it needs a little tweaking

weidazhao commented 8 years ago

Hi Ben,

I spent some time today and figured out the issue. The ServiceFabricMiddleware is registered as the first middleware in the pipeline, even before the built-in RequestServicesContainerMiddleware (which is sort of by design). So when you request an instance of ServiceFabricFeature in ServiceFabricMiddleware, the instance doesn't come from the scoped service provider that is constructed by RequestServicesContainerMiddleware, even the ServiceFabricFeature is declared to be a scoped service.

I need to talk to ASP.NET folks to review the design since the fix would have to use some internal types.

@benaadams

weidazhao commented 8 years ago

Hi David,

Ben and I are looking into how to remove the usage of HttpContextAccessor from the ServiceFabricMiddleware.

The problem is that the ServiceFabricMiddleware is registered via IStartupFilter so it will be placed before any other middlewares in the pipeline, even before the RequestServicesContainerMiddleware that provides RequestServices. Therefore there is no way for ServiceFabricMiddleware to correctly get a scoped service instance.

Do you think it is reasonable to change the logic at https://github.com/aspnet/Hosting/blob/dev/src/Microsoft.AspNetCore.Hosting/Internal/WebHost.cs#L163 to ensure RequestServicesContainerMiddleware always to be the first middleware in the pipeline?

@davidfowl

weidazhao commented 8 years ago

Actually the fix would be better at https://github.com/aspnet/Hosting/blob/dev/src/Microsoft.AspNetCore.Hosting/WebHostBuilder.cs#L202: moving 'services.AddTransient<IStartupFilter, AutoRequestServicesStartupFilter>();' to the end of the method and right before 'return services;' so it will be placed at the beginning of the pipeline.

@davidfowl

weidazhao commented 8 years ago

@benaadams a fix for https://github.com/aspnet/Hosting/issues/653 is on the way.

weidazhao commented 8 years ago

@benaadams , inspired by your pull request, I recently rewrote some of the APIs in Hosting and Gateway to better leverage the built-in DI system of ASP.NET Core. The APIs should be much simpler to use now. The recent changes also has absorbed the breaking changes of Service Fabric SDK 2.0. Please check it out.

I'm going to close the pull request since the change is no longer applicable to the current code base. Thanks for contributing the nice idea.

-David