Find the issue: unbounded growth

Last time, we implemented a MyCommands class to keep track of commands invoked by the MyCommandsController. Here is the MyCommands as we left it:

public sealed class MyCommands
{
    private readonly ConcurrentDictionary<string, Process> commands;

    public MyCommands()
    {
        this.commands = new ConcurrentDictionary<string, Process>();
    }

    public string Start(MyCommandParameters p)
    {
        Process myCommand = Process.Start(
            "MyCommand.exe",
            $"-useTheForce {p.useTheForce} -timeout {p.timeout} -auditMessage {p.auditMessage}");
        string id = Guid.NewGuid().ToString();
        this.commands[id] = command;
        return id;
    }

    public MyCommandResult Get(string id)
    {
        Process command;
        if (!this.commands.TryGetValue(id, out command))
        {
            return MyCommandResult.NotFound(id);
        }

        if (!command.HasExited)
        {
            return MyCommandResult.Pending(id);
        }

        return MyCommandResult.Completed(id, command.ExitCode);
    }
}

What possible issues do we have here? The first one that comes to mind is the unbounded growth of the dictionary. Note that we only add entries, never removing them. If the server process runs long enough, it will continue to use more and more memory. To address this, we’ll need some sort of eviction strategy. Before we try this, it would be wise to inject some testability into this design — I don’t trust that I will be able to correctly implement auto-cleanup of this dictionary without some tests guiding me.

First, let’s use dependency injection for the Process.Start call. It would be rather inconvenient to launch real processes during a unit test. To do this as safely as possible (remember, there are no tests yet), we can follow this approach:

1. Extract the Process.Start code into a method public static Process DoStart(MyCommandParameters p) in the class.
2. Move the DoStart method to a new internal static class MyCommand and update the reference in Start.
3. Rename MyCommand.DoStart to MyCommand.Start.
4. Add a field private readonly Func<MyCommandParameters, Process> start to the class.
5. Initialize the field in the constructor with the MyCommand.Start method.
6. Replace the reference to MyCommands.Start with this.start.

After all that, we should have the following changes:

public sealed class MyCommands
{
    private readonly Func<MyCommandParameters, Process> start;
    // ...
    public MyCommands()
    {
        this.start = MyCommand.Start;
        // ...
    }

    public string Start(MyCommandParameters p)
    {
        Process command = this.start(p);
        // ...
    }
    // ...
}

Now we just need to actually inject the dependency by adding a new constructor parameter. The resulting compiler error will remind us to do the right thing at the call site by passing the MyCommand.Start function:

public sealed class MyCommands
{
    private readonly Func<MyCommandParameters, Process> start;
    // ...
    public MyCommands(Func<MyCommandParameters, Process> start)
    {
        this.start = start;
        // ...
    }
    // ...
}

// In the call site:
    /* ... */ new MyCommands(MyCommand.Start) /* ... */

After a set of mostly mechanical changes, we are close to being able to write a test but not quite there yet. We still have a hard dependency on Process, a decidedly awkward collaborator. We will settle for simple indirection for the time being, by introducing IProcess. To do so, we can follow these steps:

1. Find and replace all mentions of Process with IProcess.
2. Use the resulting reference error to generate the public interface IProcess in a new file.
3. Fix the remaining reference errors by generating the necessary read-only properties (HasExited and ExitCode).
4. Change MyCommand from static to a sealed class.
5. Add a constructor private MyCommand(Process process) and initialize a corresponding field private readonly Process process.
6. Implement the interface IProcess on MyCommand and change the “not implemented” property stubs to pass through to this.process.
7. Change MyCommand.Start to return IProcess.
8. Wrap the return value of MyCommand.Start in new MyCommand(...).

After all this, MyCommand should be a nicely formed adapter:

internal sealed class MyCommand : IProcess
{
    private readonly Process process;

    private MyCommand(Process process)
    {
        this.process = process;
    }

    public bool HasExited => this.process.HasExited;

    public int ExitCode => this.process.ExitCode;

    public static IProcess Start(MyCommandParameters p)
    {
        return new MyCommand(
            Process.Start(
                "MyCommand.exe",
                $"-useTheForce {p.useTheForce} -timeout {p.timeout} -auditMessage {p.auditMessage}"));
    }
}

The program should now compile and have no observable behavioral differences. (You should verify this before proceeding.) We are finally ready for some tests!

using FluentAssertions;
using Microsoft.VisualStudio.TestTools.UnitTesting;

[TestClass]
public class MyCommandsTest
{
    [TestMethod]
    public void GetNonexistentReturnsNotFound()
    {
        MyCommands commands = new MyCommands(p => null);

        MyCommandResult result = commands.Get("does-not-exist");

        ShouldBe(
            result,
            "does-not-exist",
            "The command does-not-exist was not found.",
            MyCommandStatus.NotFound);
    }

    [TestMethod]
    public void GetStartedReturnsPending()
    {
        StubProcess proc = null;
        MyCommands commands = new MyCommands(p => proc = new StubProcess(p.auditMessage));
        string id = commands.Start(new MyCommandParameters { auditMessage = "pending" });

        MyCommandResult result = commands.Get(id);

        proc.Should().NotBeNull();
        proc.Message.Should().Be("pending");
        ShouldBe(
            result,
            id,
            "The command * is still running.",
            MyCommandStatus.Pending);
    }

    [TestMethod]
    public void GetDoneReturnsCompleted()
    {
        StubProcess proc = null;
        MyCommands commands = new MyCommands(p => proc = new StubProcess(p.auditMessage));
        string id = commands.Start(new MyCommandParameters { auditMessage = "done" });
        proc.HasExited = true;
        proc.ExitCode = 54321;

        MyCommandResult result = commands.Get(id);

        ShouldBe(
            result,
            id,
            "The command * completed with exit code 54321.",
            MyCommandStatus.Completed);
    }

    private void ShouldBe(MyCommandResult actual, string expectedId, string expectedDetail, MyCommandStatus expectedStatus)
    {
        actual.Id.Should().Be(expectedId);
        actual.Detail.Should().Match(expectedDetail);
        actual.Status.Should().Be(expectedStatus);
    }

    private sealed class StubProcess : IProcess
    {
        public StubProcess(string message)
        {
            this.Message = message;
        }

        public string Message { get; }

        public bool HasExited { get; set; }

        public int ExitCode { get; set; }
    }
}

This test class covers the basic cases we have already implemented. Now we need to start sketching out the behavior of the removal logic. It would be tempting to simply remove the item the first time Get is called after finishing the request. However, there are drawbacks to that approach. For one, the client may never end up calling Get, and so we would still grow without bound in that case. Additionally, the client may get unlucky and initiate the Get call but lose the response; on retry, the result would be gone. Instead, it seems more prudent to expire the result within a reasonable time period after the command exits.

Unfortunately that tricky word “time” comes up — the enemy of unit testing! However, we have a solution up our sleeves. Instead of wasting real wall clock time, we will need to have a programmable “alarm” of sorts to trigger expiration. Conveniently, I have already talked about this approach in my earlier post “Wake up!”. We just have one problem — there is no way right now to detect that process has exited without polling.

Typically in this case we would want to add an event (indeed, Process does have such an Exited event) but that expands the surface area of our port/adapter and introduces more external dependencies. We might be better served by working within the simple and already well-understood bounds we have set. Let’s instead kick off an async polling task on Start which checks HasExited and expires the result some time after it becomes true.

We are going to need a simulated delay function for this, so we will inject one more dependency into the MyCommands constructor:

public sealed class MyCommands
{
    private readonly Func<MyCommandParameters, IProcess> start;
    private readonly Func<TimeSpan, Task> delay;
    //...
    public MyCommands(Func<MyCommandParameters, IProcess> start, Func<TimeSpan, Task> delay)
    {
        this.start = start;
        this.delay = delay;
        //...
    }
    //...
}

The call sites in the product and test code should be updated accordingly:

/* ... */ MyCommands commands = new MyCommands(p => null, Task.Delay); /* ... */

So far, so good. No behavior has changed, and we now have a test seam to work with. To provide functionality for test purposes, we now need a relatively sophisticated test double with programmable behavior. For this purpose, I have come up with a Delay class that has two methods:

• RunAsync, which always “blocks” on the first call, and accumulates a delay interval up to a predetermined expiration, after which it invokes an onCompleted callback.
• Unblock, which unblocks RunAsync.

This design will allow us to pass delay.RunAsync for the delay function without tying up execution on the first call — if we used an all synchronous design, we might risk an infinite loop (as we will soon see). The onCompleted callback gives us a way to trigger some action after we have had enough “waiting” time. Code as complex as this deserves tests of its own, so here is our test tester:

[TestClass]
public class DelayTest
{
    [TestMethod]
    public void CompleteAfterUnblockZeroExpiration()
    {
        bool done = false;
        Delay delay = new Delay(TimeSpan.Zero, () => done = true);

        Task task = delay.RunAsync(TimeSpan.FromSeconds(1.0d));

        task.IsCompleted.Should().BeFalse(because: "first call always waits");
        done.Should().BeFalse();

        delay.Unblock();
        task.IsCompletedSuccessfully.Should().BeTrue();
        done.Should().BeTrue();
        delay.Total.Should().Be(TimeSpan.FromSeconds(1.0d));
    }

    [TestMethod]
    public void CompleteAfterTwoCallsOneMinuteExpiration()
    {
        bool done = false;
        Delay delay = new Delay(TimeSpan.FromMinutes(1.0d), () => done = true);

        Task task = delay.RunAsync(TimeSpan.FromSeconds(30.0d));

        task.IsCompleted.Should().BeFalse(because: "first call always waits");
        done.Should().BeFalse();

        delay.Unblock();
        task.IsCompletedSuccessfully.Should().BeTrue();
        done.Should().BeFalse();

        task = delay.RunAsync(TimeSpan.FromSeconds(30.0d));
        task.IsCompletedSuccessfully.Should().BeTrue();
        done.Should().BeTrue();
        delay.Total.Should().Be(TimeSpan.FromMinutes(1.0d));
    }

    [TestMethod]
    public void CompleteAfterThreeCallsOneHourExpiration()
    {
        bool done = false;
        Delay delay = new Delay(TimeSpan.FromHours(1.0d), () => done = true);

        Task task = delay.RunAsync(TimeSpan.FromMinutes(59.0d));

        task.IsCompleted.Should().BeFalse(because: "first call always waits");
        done.Should().BeFalse();

        delay.Unblock();
        task.IsCompletedSuccessfully.Should().BeTrue();
        done.Should().BeFalse();

        task = delay.RunAsync(TimeSpan.FromSeconds(59.0d));
        task.IsCompletedSuccessfully.Should().BeTrue();
        done.Should().BeFalse();

        task = delay.RunAsync(TimeSpan.FromMilliseconds(1001.0d));
        task.IsCompletedSuccessfully.Should().BeTrue();
        done.Should().BeTrue();
        delay.Total.Should().Be(TimeSpan.FromMilliseconds(3600001.0d));
    }
}

The implementation below passes these tests:

internal sealed class Delay
{
    private readonly TimeSpan expiration;
    private readonly Action onExpiration;
    private readonly TaskCompletionSource<bool> blocked;

    public Delay(TimeSpan expiration, Action onExpiration)
    {
        this.expiration = expiration;
        this.onExpiration = onExpiration;
        this.blocked = new TaskCompletionSource<bool>();
    }

    public TimeSpan Total { get; private set; }

    public async Task RunAsync(TimeSpan interval)
    {
        await this.blocked.Task;
        this.Total += interval;
        if (this.Total >= this.expiration)
        {
            this.onExpiration();
        }
    }

    public void Unblock()
    {
        this.blocked.SetResult(true);
    }
}

Now to use this class to write the failing test which will prove we have properly removed the item after completion:

[TestMethod]
public void ItemRemovedOnIntervalAfterCompletion()
{
    StubProcess proc = null;
    Delay delay = new Delay(TimeSpan.FromMinutes(3.0d), () => proc.HasExited = true);
    MyCommands commands = new MyCommands(p => proc = new StubProcess(p.auditMessage), delay.RunAsync);
    string id = commands.Start(new MyCommandParameters { auditMessage = "waiting" });

    delay.Unblock();

    proc.HasExited.Should().BeTrue();
    delay.Total.Should().Be(TimeSpan.FromMinutes(8.0d));

    MyCommandResult result = commands.Get(id);

    result.Status.Should().Be(MyCommandStatus.NotFound);
}

The test sets up an expectation that three minutes (of simulated time) after the process starts, it will exit. Five minutes after that, the process should be removed with no more waiting. This implementation passes the test:

public string Start(MyCommandParameters p)
{
    IProcess command = this.start(p);
    string id = Guid.NewGuid().ToString();
    this.commands[id] = command;
    Task task = RemoveAsync(id, command); // fire and forget
    return id;
}
// ...
private async Task RemoveAsync(string id, IProcess command)
{
    do
    {
        await this.delay(TimeSpan.FromMinutes(1.0d));
    }
    while (!command.HasExited);

    await this.delay(TimeSpan.FromMinutes(5.0d));
    this.commands.TryRemove(id, out IProcess ignored);
}

(It should now be clear that the delay method must not complete synchronously initially — otherwise, we may never exit the do-while loop.) We now have the basic functionality for expiration of stale entries, five (or six) minutes after completion. Assuming a bounded incoming rate (we’ll leave that as an exercise for the reader), we will no longer see unbounded growth in memory.

I’ll admit that this was a more complex problem to solve, but disciplined code changes and TDD made it eminently tractable. Even so, there are still more issues to be fixed! Come back next week for an exciting new installment.