Binär oder Text - das ist hier die Frage Gedanken zum Thema Dependency Injection

How Ninject inadvertently made the case for SOLID

Published on Wednesday, May 7, 2014 3:00:00 AM UTC in Programming

It's wonderful to use a robust, great, field-tested tool that simply works. Except when it doesn't.

A recent design change in some component I worked on involved switching the Ninject bindings for some type to request scope. The idea is to keep the same instance of a certain type around in a web application until the request is completed, thus creating a pseudo-singleton for as long as a request is processed. Ninject has a strategy to dispose instances when they are deactivated, meaning that when the request ends, the scoped binding would result in calls to IDispose.Dispose() when your bound type implements that interface. In my case, disposal of these instances was crucial, as they occupy valuable resources (connections to external systems). After the scope change however, I realized that my instances never were disposed and started investigation.

Update: Ninject.Web.Common 3.2.2 fixes this issue. If you haven't upgraded yet, go for it. It's still worth reading this article to learn about the original issue, especially because the authors decided to go for a quick fix instead of making the existing implementation more robust, as suggested below.

Setup

For starters, I reproduced the buggy behavior with a very simple setup to confirm it's an issue with Ninject and not something odd I do in my code:

  • Create a new, empty web application. Any type of ASP.NET project is sufficient, I chose an MVC application
  • Add the current (3.2.x) NuGet packages for Ninject, in particular Ninject, Ninject.Web.Common and Ninject.MVC3 (which is intended for MVC3+).
  • Create a type that implements IDisposable
  • Add a binding for that type and scope it to the request
  • Resolve the type, use it and observe that it is never being disposed of

The details of my implementation are trivial:

public class Demo : IDemo, IDisposable
{
    public string Id { get; private set; }

    public Demo()
    {
        Id = Guid.NewGuid().ToString();
        Debug.WriteLine("Created " + Id);
    }

    public void Dispose()
    {
        Debug.WriteLine("Disposed " + Id);
        Id = null;
    }
}

public interface IDemo
{
    string Id { get; }
}

For this type, I added the following binding in the (auto-generated) RegisterServices(…) method of Ninject:

kernel.Bind<IDemo>().To<Demo>().InRequestScope();

I simply constructor-injected the interface into my HomeController to output it's Id value and fired some requests. Here is the debug output:

Created 5b77768d-122a-4843-91e0-93cd7b84c611
Created 6bc83057-098d-46b5-919e-ea9279ef5957
Created f801488d-f214-4466-89c1-074b77823a1a
Created dbb28f5c-58d4-4bc6-babd-1bf4756eaa7b
Created f26b9ac5-08de-46d9-af13-2bc0a991f0c0
Created bbbc79fc-43bc-43f8-b456-08fcb93be34b
Created c821e8ee-ca29-4857-ab77-0cb3703c611d
Created 588eb856-6615-49c6-ae88-b5c1f73bfe4d

As you can see, the dispose messages never appear, meaning Dispose() is not called from HttpApplication.EndRequest as it should be (see above linked Ninject docs).

Analyzing the issue

I first tried finding the issue by simply looking at the involved code in Ninject, but failed to do so. The code involves loading types through reflection, threading aspects, weak references handling, and is distributed across multiple classes and even projects, all of which didn't really allow to find the issue by just reading the code, at least for me. So what I did was pull in the source code for all the above mentioned NuGet packages so I can build them myself and debug the issue. Here is what happens.

  • To hook into the HttpApplication.EndRequest event, Ninject has a module named OnePerRequestHttpModule, an IHttpModule implementation. That module can be registered in various ways with the runtime; the auto-generated code for MVC projects does it in a static Start() method using the built-in DynamicModuleUtility helper class from Microsoft.Web.Infrastructure.
  • In the handler for EndRequest events, this implementation gets the current HttpContext and uses it as scope to deactivate all instances of the respective kernels.
  • To tie the OnePerRequestHttpModule to the respective kernels, a special Ninject module is used: WebCommonNinjectModule. This is never loaded explicitly, but discovered through reflection when the kernel is created. This Ninject module derives from GlobalKernelRegistrationModule, which performs the actual registration of the kernel with the HttpModule, and extends that functionality by binding some default types.

The latter one is were the bug resides. Let's take a look at the two involved implementations. First the Load() method in GlobalKernelRegistrationModule:

public override void Load()
{
    GlobalKernelRegistration.RegisterKernelForType(this.Kernel, typeof(TGlobalKernelRegistry));
}

And now the derived type that registers OnePerRequestHttpModule: WebCommonNinjectModule

public override void Load()
{
#if !NET_35
    this.Bind<System.Web.Routing.RouteCollection>().ToConstant(System.Web.Routing.RouteTable.Routes);
    this.Bind<HttpContextBase>().ToMethod(ctx => new HttpContextWrapper(HttpContext.Current)).InTransientScope();
#endif
    this.Bind<HttpContext>().ToMethod(ctx => HttpContext.Current).InTransientScope();
}

Can you see the issue? The derived class overrides the Load() method and never calls the base implementation, which results in the kernel never being registered with the type. This, as a consequence, results in a failure to retrieve the kernel in the EndRequest event handler, and, ultimately, in no disposal of any request-scoped instances at all.

A Workaround until this is fixed

If this is what you came here for: a very simple workaround to fix this without changing the Ninject code base yourself is to create another (empty) module that derives from GlobalKernelRegistrationModule:

public class BlubbModule : GlobalKernelRegistrationModule<OnePerRequestHttpModule>
{
}

And:

private static void RegisterServices(IKernel kernel)
{
    kernel.Load(new BlubbModule());
    kernel.Bind<IDemo>().To<Demo>().InRequestScope();
}

This will force the registration of the kernel with the IHttpModule and hence fix handling the EndRequest event. The debug output of the above example now properly reports:

Created 5cf774dc-1aeb-4f12-a2e7-23c2f0e0be96
Disposed 5cf774dc-1aeb-4f12-a2e7-23c2f0e0be96
Created 2582f5e3-bfd1-4f23-9db6-f8b73d94b01a
Disposed 2582f5e3-bfd1-4f23-9db6-f8b73d94b01a
Created 40dd7bb8-2207-4814-b8a3-ff4304042192
Disposed 40dd7bb8-2207-4814-b8a3-ff4304042192
Created 8e92ae9c-0c0a-43b2-a571-e4d383a4e4ff
Disposed 8e92ae9c-0c0a-43b2-a571-e4d383a4e4ff

Good.

How a fix for this should look like

The obvious choice for this to fix is to simply add a call to the base implementation of Load() to the respective method in WebCommonNinjectModule. However, I want to take the chance to briefly talk about a better alternative. Most recently I had a discussion about whether the default style of implementing types should aim to design members as extensible as possible (e.g. create lots of virtual members as extension points), or not.

Leaving aside other aspects like YAGNI for the moment, I in particular am a strong advocate of designing classes as tightly closed as possible at first and only carefully open extension points later when you actually need them. The two main reasons for this mostly are that a) it's much harder and takes considerably more effort to create a proper design that allows overriding various aspects of your types, with potentially subtle issues slipping through unnoticed, and b) if types are closed at first and need to be touched to be extended, developers are forced to think more about what they intend to do and if it's really necessary (or if there is another, potentially better alternative) – I like to make people think :).

One of the fundamental concepts that you need to consider when making your types extensible in that way is the open/closed principle, one of the SOLID principles. It says that types should be open for extensibility, but closed for modification. When this is discussed in the context of inheritance, another closely related principle then is the Liskov substitution principle, which basically says that swapping a type of a derived type must not altering the behavior of an program (why are they related? Because if you violate the open/closed principle and modify a derived type, it's likely that replacing existing types by that derived type also does not result in equivalent behavior anymore).

During the course of the mentioned discussion I tried to find examples that illustrate the dangers of designing types with arbitrary numbers of virtual members, and the potentially severe bugs that are introduced by violating the open/closed principle. And frankly, I could not make up a better example for this than the above described bug in Ninject:

  • The (only) intention of the GlobalKernelRegistrationModule type is to have a base implementation which ensures the registration of a kernel with the generic type argument of that class.
  • Hence, that registration is the one essential feature that must stay intact in any case when people created derived classes.
  • Yet, by exposing that crucial behavior as a virtual method, GlobalKernelRegistrationModule directly depends on the goodwill of derived types to actually call the base implementation which, as demonstrated, has a huge potential to introduce subtle but severe bugs by accident, and effectively disable that required functionality completely.

So what would be a good interpretation of the open/closed principle in this case?

  • Seal the base implementation in GlobalKernelRegistrationModule so it cannot be altered by derived types in any case.
  • Create a new extension hook that allows adding more functionality by interited types, without the danger of changing the base implementation.

A typical pattern for this would be something like the following. In GlobalKernelRegistrationModule (note: you could also choose to make the extension point abstract to force its implementation):

// Seal the override => no derived type will be able to override this
public sealed override void Load()
{
    GlobalKernelRegistration.RegisterKernelForType(this.Kernel, typeof(TGlobalKernelRegistry));
    InternalLoad();
}

protected virtual void InternalLoad() { }

I had to choose the odd name "InternalLoad" because other names like "OnLoad" were taken by the base implementation already.

Then, in WebCommonNinjectModule, you would need to change the override to avoid compile-time errors:

protected override void InternalLoad()
{
#if !NET_35
    this.Bind<RouteCollection>().ToConstant(RouteTable.Routes);
    this.Bind<HttpContextBase>().ToMethod(ctx => new HttpContextWrapper(HttpContext.Current)).InTransientScope();
#endif
    this.Bind<HttpContext>().ToMethod(ctx => HttpContext.Current).InTransientScope();
}

There you go. You created a flexible hook for extensibility, and at the same time prevented anyone from (accidentally) modify your base implementation. The world's a better place now! :)

Note: This post is a follow-up on my initial bug report on GitHub.

Tags: Bug · Ninject · Open-closed Principle · SOLID