autofac / Autofac.Owin

OWIN integration for Autofac
MIT License
23 stars 15 forks source link

OwinLifetimeScopeKey configurable #1

Closed brockallen closed 8 years ago

brockallen commented 9 years ago

Is there any way we can make the OwinLifetimeScopeKey configurable and/or allow for some sort of prefix?

We have a scenario where 2 different copies of Autofac are being used in the same OWIN pipeline. Given that they're using the same key for storing per-request data in the OWIN environment then we get cross-over behavior and undesirable results. Obviously the MiddlewareRegisteredKey would also be affected by this.

I can provide more contextual information if desired. Thanks.

tillig commented 9 years ago

Is there a way to avoid doing what you're doing?

There are actually two different keys in the OWIN middleware - one key to register with the app to say, "The OWIN middleware is registered!" to avoid multiple copies of the middleware in one app; and the other to indicate the lifetime scope metadata.

That constant indicating the lifetime scope then is used in the MVC and Web API OWIN middleware to retrieve the single request lifetime scope and use it in either pipeline.

In general, I'd say running multiple copies of Autofac in the same application is... well, if not unsupported, at least really fringe. Trying to configure the key would mean you'd also have to configure the key in the other middleware components (MVC/Web API integration). I think it could get pretty confusing pretty quickly.

With ASP.NET 5 / DNX coming down unifying the whole model, trying to use multiple containers or different versions in the same app will be even more difficult, if not impossible.

brockallen commented 9 years ago

Well, long story short -- no, I can't avoid it. I'm using autofac within my own middleware, and ilmerging all of my dependencies into my own assembly. If a host then uses my middleware and then their own copy of autofac, then this situation arises. Since i'm building middleware, I can't control what the host does.

And I guess that doesn't really describe it all... but the point is that I use autofac internally as an implementation detail of my middleware. I configure it separate from anything else the host is doing.. but of course the two copies make the assumption that they're the only one in the request pipeline. I just need to be able to tweak the key used for the OWIN dictionary so my copy doesn't interfere with any others.

tillig commented 9 years ago

I don't think at this time there's a plan to enable this to happen. We always welcome pull requests, but it'll be one for each integration library; or you can fork and make your own Autofac middleware.

I'll update this to be an enhancement request, but for now... there's no out of the box way to do what you're looking to do. Perhaps not ILMerging everything together?

brockallen commented 9 years ago

It's ok, there's a lot more context I was trying to spare you. I'll just fork and rename the constants. If it's easy enough to make it configurable then I'll send a PR. Thx.

tillig commented 9 years ago

Clarification: When you want multiple copies of the middleware to run, does that mean you also want multiple request lifetimes - one per middleware copy? Or would it be multiple middleware instances with only one request lifetime shared between all the middleware instances?

huysentruitw commented 9 years ago

The problem is, by embedding Autofac in a third party middleware (Autofac A), the Autofac used in the application (Autofac B) using that third party middleware will never be considered the same. This means, if Autofac A creates a LifetimeScope, it will never be cast-able to the ILifetimeScope from Autofac B. That is what the exception referenced above is all about.

So, I think, that treating them separate is the only way this can work, right?

tillig commented 9 years ago

I don't actually know (without creating a repro). I would think that binding redirects might enable something like that to work, but... thinking about it, you may be totally right - that each middleware would HAVE to use its own lifetime scope.

Thinking this through out loud, would that mean Autofac.WebApi.Owin would always get the lifetime scope from the external/non-merged version of Autofac?

huysentruitw commented 9 years ago

It depends who first called to UseAutofacMiddleware. If the 3rd party middleware called it first, then your application will fail to get the lifetimescope because the lifetimescope of the 3rd party middleware cannot be casted to ILifetimeScope from the application (the types do not match because they live in a separate assembly). If you application was the first, then the 3rd party middleware will fail to get the lifetimescope for the same reason.

brockallen commented 9 years ago

I want them completely isolated and ignorant of one another.

-Brock

On May 14, 2015, at 4:55 PM, Travis Illig notifications@github.com wrote:

Clarification: When you want multiple copies of the middleware to run, does that mean you also want multiple request lifetimes - one per middleware copy? Or would it be multiple middleware instances with only one request lifetime shared between all the middleware instances?

— Reply to this email directly or view it on GitHub.

tillig commented 9 years ago

Still wrapping my head around this and making sure if changes get committed that we're getting the desired outcome...

In the common use case, the changes here shouldn't have any effect. In the use case for this issue in particular, does the fix solve the problem and not break other OWIN integration packages (e.g., Web API)? Or is there a need for modification in the other OWIN integration packages? I don't have the ability to test it in both scenarios.

brockallen commented 9 years ago

These two lines are all I needed to change to get what I needed:

https://github.com/IdentityServer/IdentityServer3/commit/06ab9cf55996444c89f1aa2db61aba8646ebcf8c#diff-c66cacd3da436a96f028fa1d340ddb18R45

https://github.com/IdentityServer/IdentityServer3/commit/06ab9cf55996444c89f1aa2db61aba8646ebcf8c#diff-0b0ce173b47df5f456dfbf8f5c1a1e1dR33

So I was originally asking to have these possible as a config option on the MW, that's all. So yes, in the common case it would not affect anyone using this in the recommended approach. But if someone (such as ourselves) need to have two copies of autofac in the owin pipeline that knew nothing about one another then this fix would allow for that. Each copy would be its own DI container and have no relation to any other copies of autofac in the same OWIN pipeline.

tillig commented 9 years ago

I get that; what I'm not sure about without someone testing is: If we provide some sort of solution to that (e.g., enabling it to be configured or otherwise dynamic) does it affect the Web API or MVC OWIN integration in any way?

Looking at the Web API OWIN code, for example, it uses context.GetAutofacLifetimeScope() which makes use of the extension in the OWIN integration - which relies on that Constants.OwinLifetimeScopeKey that changed.

Does the issue "resolve itself" given the only thing that can bind to the ILMerged version of Autofac.Owin is the stuff also in the ILMerged assembly? Or is that true? Is it "first in wins?"

I don't mean to belabor the issue, it's just that we need to be sure of the behavior so we can document it and give folks an idea of what to expect and I don't have the ability at the moment to set up a repro and test the various conditions.

huysentruitw commented 9 years ago

I've setup a self-host project containing WebAPI and ThinkTecture.IdentityServer3 v1.6.1 and added a hook to beginning of OWIN pipeline, so I could inspect the context of the incoming requests (before await next()) and the return context (after await.next()):

app.Use(async (context, next) =>
{
    await next();
});

I've concluded that, as long as you keep the UseAutofacMiddleware, UseAutofacWebApi and UseWebApi functions together, there is no problem because the required properties are overridden at the right time:

owin request pipeline

It should even work if we keep the OwinLifetimeScopeKey the same (so without appending a GUID). I think the only real problem is this line: https://github.com/autofac/Autofac.Owin/blob/master/src/Autofac.Integration.Owin/AutofacAppBuilderExtensions.cs#L58

This was patched here: https://github.com/autofac/Autofac.Owin/commit/6e12426152c8925a6b8a435d826dc5af38b960dc

Anyway, adding the same GUID to the OwinLifetimeScopeKey can be useful for debugging.

I also tried to setup a solution with 2 projects, each using different Autofac libraries but I failed miserably. However in my self-host project with WebAPI and IdentityServer3, I was able to call actions on both API controllers (the one from the app and the one inside IdentityServer3).

tillig commented 9 years ago

OK, PR #2 was merged and published to the Autofac MyGet feed at https://www.myget.org/F/autofac/ as version 3.1.1-CI-216.

Can you test it out and make sure it works as expected? If it does, I can push to NuGet as official 3.1.1.

brockallen commented 9 years ago

I've been traveling -- I'll try to get to this on the weekend or next week. Thx

tillig commented 9 years ago

@brockallen Just wanted to follow up on this to see if you had a chance to test it. If it's good, I'll push to NuGet.

huysentruitw commented 9 years ago

I would not push this because it still doesn't fix it all, see https://github.com/autofac/Autofac.Owin/pull/2#issuecomment-108780348

tillig commented 9 years ago

Hmmm. Open to suggestions. :persevere:

huysentruitw commented 9 years ago

I just removing the AutofacMiddelwareRegistered-check and did some tests.

My tests show that you can hook up as many Autofac middlewares (with that check removed) as you want (talking about WebAPI only here), as long as everything is added in the right sequence (UseAutofacMiddleware, UseAutofacWebApi, UseWebApi, UseAutofacMiddleware, UseAutofacWebApi, UseWebApi, etc) to the OWIN pipeline, the autofac:OwinLifetimeScope gets overridden at the correct time (right before it is consumed by the UseWebApi middleware) and it just works.

I already asked @alexmg why he added that check here: https://github.com/autofac/Autofac.Owin/commit/6e12426152c8925a6b8a435d826dc5af38b960dc#commitcomment-11520276

So, removing that check can only break software that register the middleware more than once by accident.

brockallen commented 9 years ago

Sorry for dragging my feet on this. We did run into another issue related to our use of autofac, but given our fork I was able to work around it. I'll follow up next week when I return home from travel.

-Brock

On Jun 15, 2015, at 5:11 PM, Travis Illig notifications@github.com wrote:

Hmmm. Open to suggestions.

— Reply to this email directly or view it on GitHub.

tillig commented 9 years ago

@brockallen Did you try this out?

We're working on getting the DNX support out and everything's revving to version 4.0.0 so this would be a good time to add stuff like this. I can remove that defensive check at https://github.com/autofac/Autofac.Owin/commit/6e12426152c8925a6b8a435d826dc5af38b960dc#commitcomment-11520276 if that will help. I can then issue a 4.0.0-beta for testing.

brockallen commented 9 years ago

Unfortunately no -- I certainly meant to make time. My failure.

Related to this is this issue in ASP.NET 5 which suggest that we do need to be able to configure different branches in the pipeline with different DI: https://github.com/aspnet/Home/issues/242

tillig commented 9 years ago

Check out Autofac.Owin 4.0.0-CI-221 on the MyGet (https://www.myget.org/F/autofac/) and let me know what you think. It includes PR #3 as well, which allows separating the registration of the Autofac lifetime scope injection from middleware registered with Autofac.

I'm having a tough time keeping up on all the changes and requirements for DNX and ASP.NET 5, so suggestions/pointers are welcome. I spend a good lot of time just trying to keep the build stable from beta to beta... plus the other integration libraries that need to be updated for compatibility. (PRs welcome! :smile:)

brockallen commented 9 years ago

np -- thanks and i'll see if it can help us.

tillig commented 9 years ago

I've pushed a version to NuGet - 4.0.0-beta7-222 - that contains the latest changes including PR #3.

tillig commented 8 years ago

Just wanted to follow up here and see if this helped. Given it may tie into Identity Server issue #2117 I thought I'd add the reference and see if there was anything left to do here.

If not, I'll close it up and call it good.

brockallen commented 8 years ago

I don't think it's related.

This issue here was essentially that we needed to have an isolated sub-container unrelated to any other code running in the OWIN pipeline. We're still working with a fork that's embedded into IdSvr (and thus internalized). The original pushback (as far as I remember and understand) from you was that you felt that any host using Autofac should be using a shared Autofac config and container. We feel differently because IdSvr is meant to be self-contained and encapsulates its work. Also, it doesn't help that OWIN itself doesn't define any standard DI.

This logical issue persists in ASP.NET 5, BTW: https://github.com/aspnet/DependencyInjection/issues/246

tillig commented 8 years ago

Fair enough. The reason I thought it might be related was that, if you expose control of configuring nested containers like the one in Identity Server, some of the things may need to "leak out" to give access to folks to hook into. I can see a plugin developer wanting to get the request lifetime scope from a component used within Identity Server and having that context key available in some form so the right scope is grabbed would be important.

If it's not related, it's not related. No big deal.

Either way I'd love to hear if this is fixed so we can potentially close it.

brockallen commented 8 years ago

I suspect we can close this. I do appreciate your time.

tillig commented 8 years ago

No problem. Let me know if there's anything else we can do.