I’m working on getting an older .NET Framework solution upgraded from .NET 4.6.1 to 4.8. While doing that I’m also switching it from packages.config style to PackageReference style and updating the NuGet dependencies to the latest versions.
I suppose the first question is, why do that?
The main driver is a colleague needs to consume a NuGet package targetting .NET Standard 2.0 and that works best on .NET 4.7.2+ (on that link read the small print below the “.NET implementation support” table). If we’re going to take on the effort to upgrade, we might as well jump all the way to .NET 4.8 right? Taking that logic a bit further, if we’re going into an upgrade cycle, and need to re-test everything, what other wishlist/upgrade-y sort of items should we also tackle?
Moving to PackageReferences is really about cleanup. The main thing that’s nice about PackageReference style is your project only needs to reference the package you want and its child dependencies are just included automatically (transitive dependencies). With packages.config all the dependencies are referenced so you quickly end up with a giant (usually unmanageable) mess if you aren’t careful. You also have to maintain two files (packages.config & csproj) which is never fun. When you move to PackageReference make sure you also turn on AutoGenerateBindingRedirects (for console apps and test projects) because you then get to clean up the confusing assembly binding redirects in your config files. What will happen is the final “.config” in your “/bin/” will have all that stuff added automatically. The net result of these changes is that our old-school projects feel and operate more like the .NET Core (SDK style) projects. If you aren’t already using the SDK style, you will learn to love it I promise you. Need I say more? OK I will: You also get a nice NuGet icon in Visual Studio when browsing references. An icon! Sold yet?
Upgrading NuGet packages is really just about getting bug fixes and features. Most devs prefer to be on the latest and greatest and we want to keep the devs happy too. It’s bad enough we’re asking them to work on .NET Framework, let alone really old .NET Framework!
Approaching this upgrade is a task. Visual Studio has a migration tool, but I prefer to remove all the dependencies and then one-by-one add back in what we are actually using. I do that because it’s an opportunity to prune stuff that has been removed from the code but not cleaned up from artifacts. Everything went pretty smoothly with the exception of Unity.
Here’s the version of Unity we were on:
<package id="Unity.AspNet.WebApi" version="5.0.15" targetFramework="net461" /> <package id="Unity.Interception" version="5.8.11" targetFramework="net461" /> <package id="Unity.Mvc" version="5.0.15" targetFramework="net461" /> <package id="Unity.RegistrationByConvention" version="5.8.11" targetFramework="net461" />
There were a lot more Unity references in packages.config, but that’s the stuff we’re actually using.
Upgrading to:
<PackageReference Include="Unity.AspNet.WebApi" Version="5.11.1" />
<PackageReference Include="Unity.Interception" Version="5.11.1" />
<PackageReference Include="Unity.Mvc" Version="5.11.1" />
<PackageReference Include="Unity.RegistrationByConvention" Version="5.11.1" />
Unity Container is pretty liberal with its upgrades. You will get breaking changes. Expect compilation failures due to the namespaces being rearranged. I won’t get into that stuff here, though, because it’s pretty easy to work through. What I want to share are the more difficult runtime breaks (behavior changes) that might be more surprising. Hopefully Google will index this and someone’s day will be saved. You are welcome, friend!
Problem 1: Internal constructors being used that were excluded before.
This codebase is new to me and has a lot of stuff that looks like this:
public interface IService {}
public class Service : IService
{
private readonly object _Context;
public Service()
: this(new object())
{
}
internal Service(object context)
{
_Context = context;
}
}
Worked fine before but after the upgrade we started to see a whole lot of (easily reproducible) null reference exceptions coming from the classes using this pattern. Setting some breakpoints on the before and after source, we figured out that Unity is calling the internal ctor on the new version instead of the public ctor the old version used.
Digging into the Unity source on GitHub, here’s how the new version selects its ctor:
protected override IEnumerable<ConstructorInfo> DeclaredMembers(Type type)
{
return type.GetTypeInfo()
.DeclaredConstructors
.Where(ctor => !ctor.IsFamily && !ctor.IsPrivate && !ctor.IsStatic);
}
And here’s how the old version worked:
ConstructorInfo[] constructors = typeToConstruct.GetTypeInfo()
.DeclaredConstructors
.Where(c => c.IsStatic == false && c.IsPublic)
.ToArray();
Can you spot the difference?
A subtle change, but in our case it is significant. internal ctor is IsPublic=false and IsPrivate=false. The new version will choose internal ctors where the old version excludes them. Wow Unity! Big change.
Let’s take a step back. Does the class above use good design patterns? No, it doesn’t look right, does it? If anything, I would expect these constructors to be reversed. A public one for injection and an internal one for boot-strapping dependencies, likely to help in writing unit tests. But I’m new to this codebase and there’s a lot of these objects. Going and making a bunch of changes in stable code to accommodate a NuGet upgrade the business doesn’t care about probably won’t go over too well, so my goal right now is to make the new Unity behave like the old one. I’ll work on fixing the patterns being used over the coming sprints.
So how do we restore the old behavior? Good news and bad news. Unity is pretty extensible, but the documentation is severely lacking. I think there’s a way to plug in a policy and make the selection work the way we want, but I couldn’t figure it out. I decided just to swap out the default ConstructorProcessor with my own derived type that overrides the one bit of code above. Please leave a comment if you are a Level 10 Unity Wizard possessing the magic to do this without reflection.
Here’s how that looks:
using System;
using System.Collections.Generic;
using System.Reflection;
using Unity;
using Unity.Extension;
using Unity.Processors;
using Unity.Builder;
using Unity.Storage;
using Unity.Policy;
namespace DependencyInjection
{
/// <summary>
/// Unity 5.8.11 would not choose 'internal' constructors but Unity 5.11.1 will include them in its candidate list.
/// This extension restores the legacy behavior so that all code relying on it will continue to function.
/// </summary>
public class UnityLegacyConstructorResolutionExtension : UnityContainerExtension
{
protected override void Initialize()
{
FieldInfo stagesField = typeof(StagedStrategyChain<MemberProcessor, BuilderStage>).GetField("_stages", BindingFlags.Instance | BindingFlags.NonPublic);
FieldInfo policySetField = typeof(MemberProcessor<ConstructorInfo, object[]>).GetField("_policySet", BindingFlags.Instance | BindingFlags.NonPublic);
IList<MemberProcessor>[] stages = (IList<MemberProcessor>[])stagesField.GetValue(Context.BuildPlanStrategies);
IList<MemberProcessor> constructorProcessors = stages[(int)BuilderStage.Creation];
MemberProcessor defaultConstructorProcessor = constructorProcessors[0];
constructorProcessors.Clear();
IPolicySet policy = (IPolicySet)policySetField.GetValue(defaultConstructorProcessor);
UnityContainer container = (UnityContainer)Context.Container;
LegacyConstructorProcessor constructorProcessor = new LegacyConstructorProcessor(policy, container);
policy.Set(typeof(ISelect<ConstructorInfo>), constructorProcessor);
Context.BuildPlanStrategies.Add(constructorProcessor, BuilderStage.Creation);
}
private class LegacyConstructorProcessor : ConstructorProcessor
{
public LegacyConstructorProcessor(IPolicySet policySet, UnityContainer container)
: base(policySet, container)
{
}
protected override IEnumerable<ConstructorInfo> DeclaredMembers(Type type)
{
if (type == null)
throw new ArgumentNullException(nameof(type));
return type.GetConstructors(BindingFlags.Public | BindingFlags.Instance);
}
}
}
}
Lot’s of reflection to do something that ought to be simple, and likely might be. Anyway, register that extension with your container and you should be good to go. Something like this:
public static IUnityContainer UnityContainer { get; } = new UnityContainer()
.AddNewExtension<UnityLegacyConstructorResolutionExtension>();
Problem 2: PerRequestLifetimeManager exceptions being thrown.
The other problem we’re having is suddenly these exceptions are popping up in our logs:
throw new InvalidOperationException(
"The PerRequestLifetimeManager can only be used in the context
of an HTTP request.Possible causes for this error are using the
lifetime manager on a non-ASP.NET application, or using it in a
thread that is not associated with the appropriate
synchronization context.");
Coming from the Unity.Mvc package, PerRequestLifetimeManager.
This one was more tricky because the old version and the new version look identical.
Consider this code:
HttpContext current = HttpContext.Current; // current = A valid context. await CallSomeLogicAsync(current).ConfigureAwait(false); current = HttpContext.Current; // current = null IService service = DependencyResolver.Current.GetService<IService>();
The important thing here is the ConfigureAwait(false). After that, if our code resumes on some other thread, HttpContext is gone (null). But that has always been the case, why is DependencyResolver.Current.GetService suddenly throwing where it returned null before?
Stepping back again, this is really a bug in our code. That call just returns null so it’s pretty pointless. But the same pattern is all over the place! Someone might be reliant on that null, I have no way of knowing. What I need to do right now is restore the behavior in Unity. That will buy me some time to fix everything up in a more controlled roll-out.
The Unity.Mvc UnityDependencyResolver (both old and new) will return null if a ResolutionFailedException is thrown:
public object GetService(Type serviceType)
{
if (typeof(IController).IsAssignableFrom(serviceType))
{
return _container.Resolve(serviceType);
}
try
{
return _container.Resolve(serviceType);
}
catch (ResolutionFailedException)
{
return null;
}
}
So something under the hood must have changed what exception is bubbling up right?
Digging into the Unity source on GitHub again, here’s how the new version handles resolution exceptions:
catch (Exception ex)
{
context.RequiresRecovery?.Recover();
if (!(ex.InnerException is InvalidRegistrationException) &&
!(ex is InvalidRegistrationException) &&
!(ex is ObjectDisposedException) &&
!(ex is MemberAccessException) &&
!(ex is MakeGenericTypeFailedException))
throw;
throw new ResolutionFailedException(context.RegistrationType, context.Name, CreateMessage(ex), ex);
}
And here’s how the old version worked:
catch (Exception ex)
{
context.RequiresRecovery?.Recover();
throw new ResolutionFailedException(context.OriginalBuildKey.Type,
context.OriginalBuildKey.Name, ex, context);
}
This looks to me like a bug fix in Unity. It really should have been throwing the whole time because someone is asking for resolution without the request context being available. The throw is good because it notifies us that there is a problem in our code! But for now, let’s put back the old behavior.
All we need to do is change the throw from an InvalidOperationException to a ResolutionFailedExcetpion so let’s tweak the Unity code:
using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using Unity;
using Unity.Lifetime;
namespace DependencyInjection
{
/// <summary>
/// This class comes from the Unity.Mvc package so that we can swap in our own version of UnityPerRequestHttpModule.
/// </summary>
public class UnityPerRequestLifetimeManager : LifetimeManager, IInstanceLifetimeManager, IFactoryLifetimeManager, ITypeLifetimeManager
{
private readonly object _LifetimeKey = new object();
public override object GetValue(ILifetimeContainer container = null)
=> UnityPerRequestHttpModule.GetValue(_LifetimeKey);
public override void SetValue(object newValue, ILifetimeContainer container = null)
=> UnityPerRequestHttpModule.SetValue(_LifetimeKey, newValue);
public override void RemoveValue(ILifetimeContainer container = null)
{
IDisposable disposable = GetValue() as IDisposable;
disposable?.Dispose();
UnityPerRequestHttpModule.SetValue(_LifetimeKey, null);
}
protected override LifetimeManager OnCreateLifetimeManager() => new UnityPerRequestLifetimeManager();
}
/// <summary>
/// UnityContainer 5.8.11 turns InvalidOperationException into ResolutionFailedException so GetDictionary doesn't throw if no HttpContext is available.
/// This class restores that feature for UnityContainer 5.11.5 by throwing a ResolutionFailedException directly.
/// </summary>
public class UnityPerRequestHttpModule : IHttpModule
{
private static readonly object s_ModuleKey = new object();
private static Dictionary<object, object> GetDictionary(HttpContext context)
{
if (context == null)
{
throw new ResolutionFailedException(
typeof(UnityPerRequestLifetimeManager),
null,
"The PerRequestLifetimeManager can only be used in the context of an HTTP request.Possible causes for this error are using the lifetime manager on a non-ASP.NET application, or using it in a thread that is not associated with the appropriate synchronization context.");
}
Dictionary<object, object> dict = (Dictionary<object, object>)context.Items[s_ModuleKey];
return dict;
}
internal static object GetValue(object lifetimeManagerKey)
{
Dictionary<object, object> dict = GetDictionary(HttpContext.Current);
if (dict != null)
{
if (dict.TryGetValue(lifetimeManagerKey, out object obj))
{
return obj;
}
}
return LifetimeManager.NoValue;
}
internal static void SetValue(object lifetimeManagerKey, object value)
{
Dictionary<object, object> dict = GetDictionary(HttpContext.Current);
if (dict == null)
{
dict = new Dictionary<object, object>();
HttpContext.Current.Items[s_ModuleKey] = dict;
}
dict[lifetimeManagerKey] = value;
}
/// <summary>
/// Disposes the resources used by this module.
/// </summary>
public void Dispose()
{
}
/// <summary>
/// Initializes a module and prepares it to handle requests.
/// </summary>
/// <param name="context">An <see cref="HttpApplication"/> that provides access to the methods, properties,
/// and events common to all application objects within an ASP.NET application.</param>
public void Init(HttpApplication context)
{
if (context == null)
throw new ArgumentNullException(nameof(context));
context.EndRequest += OnEndRequest;
}
private void OnEndRequest(object sender, EventArgs e)
{
HttpApplication app = (HttpApplication)sender;
Dictionary<object, object> dict = GetDictionary(app.Context);
if (dict != null)
{
foreach (IDisposable disposable in dict.Values.OfType<IDisposable>())
{
disposable.Dispose();
}
}
}
}
}
We add that to our code and then update our application startup to register the IHttpModule:
// Uses modified version of the Unity.Mvc UnityPerRequestHttpModule. DynamicModuleUtility.RegisterModule(typeof(UnityPerRequestHttpModule));
And then we update our registrations to use UnityPerRequestLifetimeManager instead of PerRequestLifetimeManager.
// Uses modified version of the Unity.Mvc PerRequestLifetimeManager. container.RegisterType<IService, Service>(new UnityPerRequestLifetimeManager());
Hey, I never said it was going to be pretty!