Whole day of yesterday I’ve spent debugging some “random” error that came up third time during this week. The issue did go away after some tribal dances around the code, but two other instances did not get resolved easier. The biggest problem was that only one of the developers on the team did get the issue, and everybody else were not affected. And the same happened in the new deployment. And the issue was quite major – the whole site did not load. Nothing beyond the login screen. And it was impossible to hook in the debugger, it was bypassing all the breakpoints.

I would’ve gone through the issue and try debugging it, by my machine was not affected by the problem. I could not reproduce the issue locally. But on my home machine the issue came up. Strange I through, but was glad it came up – means I can dig my teeth deep in the problem and debug it.

Not going to tell you all the details about debugging session, but the resolution is worth of your attention. It turned out that one of our controllers (we are talking about Asp.Net MVC4 application) had a constructor that did not just received the dependencies, as it should by DI rules, but also done some work. Here is the controller:

    public ShipmentsController(
        IQueryHandler<FindEntityQuery<Shipment>, Shipment> shipmentLocatorHandler, 
        IQueryHandler<AllEntitiesQuery<Shipment>, IEnumerable<Shipment>> shipmentsList, 
        IQueryHandler<FindMinorEquipmentForSiteQuery, IEnumerable<MinorEquipmentType>> findMinorEquipmentForSite, 
        IQueryHandler<FindEntityQuery<ShipmentItem>, ShipmentItem> findShipmentItem, 
        IQueryHandler<FindAssetsForSiteQuery, IEnumerable<Asset>> findAssetsForSite, 
        IQueryHandler<FindEntityQuery<Asset>, Asset> findAsset, 
        IQueryHandler<FindAssetsSerialNumbersAtSiteQuery, IEnumerable<string>> findSerialNumbers, 
        IQueryHandler<FindAssetsDistinguishingNumberAtSiteQuery, IEnumerable<string>> findDistinguishingNumbers, 
        IQueryHandler<FindFilteredAssetsForSiteQuery, IEnumerable<Asset>> findFilteredAssetsForSite, 
        IQueryHandler<ShipmentAssetsWithClashingMovementsQuery, ShipmentAssetMovementClashes> shipmentAssetsWithClashes, 
        IQueryHandler<FindShipmentExchangeRateQuery, ShipmentExchangeRate> shipmentExchangeRate, 
        IQueryHandler<FindEntityQuery<Tuple<int, bool>>, Tuple<int, bool>> shipmentSent, 
        IQueryHandler<FindShipmentFreeTextItemQuery, ShipmentsFreeTextItem> shipmentsFreeTextItem, 
        ICommandHandler<UpdateAssetInShipmentCommand> updateAssetInShipmentCommandHandler, 
        IFileList fileList, 
        IFileDownload fileDownload, 
        IConfiguration configuration)
    {
        this.shipmentLocatorHandler = shipmentLocatorHandler;
        this.shipmentsList = shipmentsList;
        this.findMinorEquipmentForSite = findMinorEquipmentForSite;
        this.findShipmentItem = findShipmentItem;
        this.findAssetsForSite = findAssetsForSite;
        this.findAsset = findAsset;
        this.findSerialNumbers = findSerialNumbers;
        this.findDistinguishingNumbers = findDistinguishingNumbers;
        this.findFilteredAssetsForSite = findFilteredAssetsForSite;
        this.shipmentAssetsWithClashes = shipmentAssetsWithClashes;
        this.shipmentExchangeRate = shipmentExchangeRate;
        this.shipmentSent = shipmentSent;
        this.shipmentsFreeTextItem = shipmentsFreeTextItem;
        this.updateAssetInShipmentCommandHandler = updateAssetInShipmentCommandHandler;

        var uploadContainer = configuration.GetUploadDirectory();
        var fileStorageLocationWithContainer = uploadContainer + "/Customisations/images/logo.png";
        var logoFile = fileList.LatestFilename(fileStorageLocationWithContainer);
        logoFileUrl = logoFile != null
                          ? fileDownload.DownloadFileUrl(logoFile, 1440)
                          : "/content/images/Logo-Faded-Edges-Small.png";
    }

What a bloater you might say. And you will be absolutely right!! The number of dependencies is unacceptable. But this is not the problem I found. Have a look on this part:

        var uploadContainer = configuration.GetUploadDirectory();
        var fileStorageLocationWithContainer = uploadContainer + "/Customisations/images/logo.png";
        var logoFile = fileList.LatestFilename(fileStorageLocationWithContainer);
        logoFileUrl = logoFile != null
                          ? fileDownload.DownloadFileUrl(logoFile, 1440)
                          : "/content/images/Logo-Faded-Edges-Small.png";

Here we do some work in constructor. We want to give some value to logoFileUrl field, so later on that can be used by controller actions. The code itself is not too bad, but it must not be done in controller.

What can go wrong?” you might ask. Simples: exceptions can be thrown, every second line above can throw an exception. And that exactly what happened in my case. fileList.LatestFilename was throwing exception, because it could not find Azure Blob Storage container.

How did this affect only some developers? Somebody did delete all the local containers, and all failed.

How did his affect the whole site? We are using MvcSitemapProvider and when it creates the menu it walks through all controllers and creates instances of them. And of course it could not create the specific controller, because an exception was thrown in the constructor. And SiteMapProvider did swallow the original exception and threw one of it’s own with no trace of the original problem.

So, kids. Keep your constructors simple, make sure the objects can always be created.

Now my task is to investigate how we can prevent these issue happening through tests or analysis on build server. Possibly I’ll have to create a test that through reflection finds all the controllers, and tell DI to create the instances of the controllers. That sounds like a huge integration test that might take a day to write and good couple minutes to execute. Alternatively I’ll look on FxCop analysis and try to get it to throw warnings on similar issues. Any other ideas?

  • Kobi

    I’ve had a similar problem with Ninject and MVC 4. I was injecting an entity context, but with a different connection string for each user. This worked well when the user was logged in.

    When an unauthenticated request came, Ninject created an empty context. The controller was created, checked authoritarian, and redirected the request to the login page. Great.

    Then we upgraded to Entity Framework 5. The context changed – we moved from ObjectContext to DbContext – and the default constructor failed (tl;dr version).

    The result: in most controllers the request of unauthenticated users failed instead of being redirected.

    I decided to remove all run-time dependencies from my controllers (except HttpContext). I wrapped the entity context in a lazy loader, so I only open it when I must.

    I’ve also able to add unit tests (not integration tests) that make sure all controllers can be created – and also a few repositories and other services the controllers use. I only had a handful of controllers, so I added them manually as testcases [TestCase(typeof(HomeController))].

    • Different connection string per user sounds terrifying to me. Have you seen FilteredDbSet ( http://patrickdesjardins.com/blog/using-a-filtereddbset-with-entity-framework-to-have-dynamic-filtering )?

      About removing all dependencies from controllers: how do you communicate with other application layers from your controllers? do you not have repositories and use DbContext directly?.

      As for unit vs integration test to check all your controllers – in my case unit test would not pick up the issue I was battling with. And we have hundreds of controllers, so reflection must be leveraged.

      • Kobi

        I should probably clear that up:

        Connection string is not per user, of course, but per group (it’s a business/standards limitation. We’re working on that)

        What
        I meant by “removing all dependencies” is removing all “deep”
        dependencies, if that makes sense – I have repositories and other
        services that do the real work; I want to be able to create all of them.

        Usually,
        a unit test will use mocks for everything (including your IFileList),
        so you will not see an exception. These tests test the behavior of the
        controller. In this case, I have a test that uses the same bindings I
        have in production: there are no mocks (except those that Ninject needs
        for configuration).

        Naturally, most objects are created with an
        invalid configuration – but that is OK as long as I don’t do anything in
        the constructor, and that is the point of these tests: I know I can
        create all controllers. Similarly, I suppose you could make unit tests
        the use mocks that throw exceptions on all actions: the test would fail
        if the constructor does anything besides setting a field.

        However, my project is much smaller, so it is simpler to test.

        Another
        small point: Do you have an integration test for IFileList? That test should have failed, which could have saved a lot of
        trouble.

        • Ah, that sound much more sane now ! -)
          For IFileList, no it does not have any tests on it. Even worse, the method that is failing is marked [Obsolete], and still being used.

          And even if we did have an integration test for that, it should throw an exception on trying to access non-existing resource.

          Really good point about the mocks that fail on any operations. Much better way than I originally thought! Thank you!

  • Pingback: Test your constructors to be simple « Trailmax Tech()

  • Pingback: Test all you queries to have handlers « Trailmax Tech()

  • Pingback: Functional Injection Discussion « Trailmax Tech()