azzlack / Microsoft.AspNet.WebApi.MessageHandlers.Compression

Drop-in module for ASP.Net WebAPI that enables GZip and Deflate support
Apache License 2.0
194 stars 54 forks source link

HttpContext gets lost because of .ConfigureAwait(false) in SendAsync() and DecompressContent() #33

Closed martyt closed 8 years ago

martyt commented 8 years ago

I'm using your server compression package on a WebAPI 2.0 project and have run into an odd problem where HttpContext.Current becomes null in controller actions that handle decompressed request content.

After some digging on StackOverflow, it seems that the .ConfigureAwait(false) calls you're using on every await are the source of the problem. I removed all of them from your code and rebuilt and everything is working fine now.

From what I understand, the ConfigureAwait(false) call tells await not to capture the synchronization context and restore it when it resumes after the await. This has the effect that, after an await, HttpContext.Current becomes null.

If there's a specific reason you're doing .ConfigureAwait(false), I guess you won't want to make any change. But otherwise, it would be greatly helpful if you could remove them. (I'm not a GitHub pro, so submitting a suggest code change isn't something I'll be doing.)

For reference, you might take a look at Stephen Cleary's answer on this SO question: http://stackoverflow.com/questions/24956178/using-httpcontext-current-in-webapi-is-dangerous-because-of-async

azzlack commented 8 years ago

Ironically, Stephen Cleary is the reason I put those ConfigureAwait(false) in there :-) I've run into deadlocks frequently without it set, like described on Cleary's blog: http://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

Instead of using HttpContext.Current, could you use this.Request.GetOwinContext() ?

martyt commented 8 years ago

I ended up modifying my code to use Thread.CurrentPrincipal instead - (what I was really after was the identity of the current user, which is typically available at HttpContext.Current.User) - we use AutoFac for dependency injection and need access to the IPrincipal deep down in our business layer where things like Request and User aren't available, so we set up a registration with AutoFac in Global.asax.cs that would get the value for us

While I (sort of) understand Cleary's writings about deadlocks with async, I wonder whether this particular code, being a part of the pipeline, should avoid using .ConfigureAwait(false) ? It would seem critical to preserve the synchronization context (HttpContext, in this case) through the entire pipeline. But maybe the best advice is to simply not rely on HttpContext.Current in a WebAPI application.

CheloXL commented 8 years ago

If you are hosting your app in a service, HttpContext.Current doesn't exists. It's a leftover from the old asp.net. In fact, if you use asp.net core, there is no more HttpContext. I would suggest you to do whatever you do in another way...

azzlack commented 8 years ago

@martyt Getting the logged on user in OWIN/WebAPI 2 should be done by running Request.GetOwinContext().Authentication.User.

martyt commented 8 years ago

@azzlack - and so it is! (Can you tell I've spent most of my life in "old style" ASP.NET?)