U4-9604 - ManyObjectsResolverBase issue with current HttpContext, breaks health checks and more

Created by Shannon Deminick 06 Mar 2017, 16:38:22 Updated by Shannon Deminick 28 Mar 2017, 02:11:46

Subtask of: U4-9609

Sometimes when you run the health check you might get an error which is because of this code:

var useSsl = GlobalSettings.UseSSL || HealthCheckContext.HttpContext.Request.ServerVariables["SERVER_PORT"] == "443";

we need to first check if the ServerVariables collection contains that key - sometimes it doesn't.

Comments

Stephan 23 Mar 2017, 17:19:10

Can repro an error but not sure it's linked to ServerVariables not containing the variable - ServerVariables is a NameValueCollection which should returns null if the key is not found - but the error comes when trying to read server variables in IIS.


Stephan 23 Mar 2017, 17:24:58

Also in ExcessiveHeadersCheck when getting server variables.


Stephan 23 Mar 2017, 18:20:08

In addition seeing some weird things with HealthCheckResolver - each health check runs its own request, but the resolver instanciate all checks, just to return one of them each time = sub-optimal, perfs-wise.


Stephan 24 Mar 2017, 09:56:58

Ouch... we seem to have an ugly issue in ManyObjectsResolverBase with HttpContext.Current being captured in a weird way ;(


Stephan 24 Mar 2017, 10:13:01

Root cause of the issue was that ManyObjectsResolverBase somehow captured the current HttpContext ''at the time the resolver was created'' and then many weird things could happen when the resolver's scope was ObjectLifetimeScope.HttpRequest, including reusing the same objects in many requests (thus leading to, I believe, the weird ServerVariables errors). Just set a breakpoint in HealthCheckResolver and marvel at the fact that the resolvers, which are meant to be per-request, are actually only created once.

The subtle thing is that some health checks trigger an application restart (trying to write files in App_Code or bin) and thus, various HttpContext would be captured during restarts, leading to a very confusing picture (ie it would not seem to consistently fail).

So... changing the title of this issue to reflect the real issue.

PR https://github.com/umbraco/Umbraco-CMS/pull/1825

Fixes the ManyObjectResolverBase - review: now, a new set of health checks should be created each time the health checks run (can see eg with a breakpoint) and there should not be ServerVariables errors anymore.

Assiging to @Shandem for review as it touches Core's resolvers.


Shannon Deminick 27 Mar 2017, 03:29:06

@zpqrtbnk makes sense, good find! I've added a comment in the PR, we just need to put a null check back in. Once you do that you can merge in the PR and close the issue.


Stephan 27 Mar 2017, 06:52:24

Updated, back in review & assigned to you.


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.5.12

Sprint: Sprint 55

Story Points: 1

Cycle: