autofac / Autofac.Mvc

ASP.NET MVC integration for Autofac
MIT License
49 stars 23 forks source link

Resolves #20 - NullReferenceException when HttpContext.Current is null #21

Closed craigfowler closed 7 years ago

craigfowler commented 7 years ago

This pull request will resolve #20 - RequestLifetimeScopeProvider.EndLifetimeScope can throw NullReferenceException if HttpContext.Current is null (such as when the HTTP request makes use of only OWIN middleware and does not pass through the regular MVC stack).

tillig commented 7 years ago

Would it be better to put the HttpContext.Current check in the LifetimeScope getter/setter? That's currently the only property that touches HttpContext.Current and it looks like EndLifetimeScope checks for a null scope before trying to dispose of it.

craigfowler commented 7 years ago

@tillig - I did consider that, but then we'd end up with a property for which its get/set are not parallel in their semantics.

Sure, when HttpContext.Current is null, then the getter would return null, but the setter would logically have to do one of the two following:

I considered introducing that discrepancy to be a greater evil than introducing a duplicate check on the HttpContext.Current elsewhere.

That said - it's a borderline case, so I'm happy if you'd prefer it pushed into the property getter/setter. An alternative is to do away with that static property altogether and replace it with separate get/set methods, which might make it clear that it's not expected to "just behave like a normal C# property".

tillig commented 7 years ago

I do see this line in GetLifetimeScope is actually also using a specific HttpContext.Current check so maybe checking in the EndLifetimeScope is fine. I'm not sure I feel strongly about it one way or the other. Given it's all internal I suppose we could refactor it later if needed.