Testing Threading

Asynchronen Code zu testen ist schwierig. Umso leichter haben es Bugs sich dort einzurichten wo man schlecht mit dem Besen hinkommt. Ein kurzer Bericht aus dem Projektalltag.

In manchen Projekten haben wir es auf Kundenseite glücklicherweise mit fähigen Administratoren zu tun die uns schon oft bei der Fehlersuche geholfen haben.
Erst letzter bin ich auf einen Bug hingewiesen worden der mit dem konkurrierenden Zugriff auf eine Collection zu tun hatte.

Einfach ausgedrückt gibt es eine Komponente die Tickets parallel in mehreren Worker-Threads abarbeitet. Die Zuteilung der Tickets übernimmt ein Buchhalter-Thread, gehalten werden die Tickets in einer Oracle-Tabelle. Da die Threads auf der starken Kundenhardware sehr schnell die Tickets aktualisieren haben wir Probleme mit dem Oracle Connection Manager bekommen - die Connections wurden massenweise immer wieder ungültig und mussten zurückgesetzt werden (80% CPU-Last bei 5% Insert-Last!).

Um die Datenbank zu entlasten habe ich daher eine Remote Facade davorgesetzt sodass die vielen schnellen Updates sich im Hauptspeicher akkumulieren konnten bevor sie in einem Block propagiert wurden. Einzig Insert & Delete wurden direkt propagiert. Allerdings vergass ich, diese Operationen zu synchronisieren und so konnte es vorkommen dass der Buchhalter-Thread ein abgearbeitetes, gerade im Löschen begriffenes Ticket erneut zuteilte.

Vor das Bugfixing hat der liebe Gott namens Kent Beck die Reproduktion des Fehlers in Form eines Unit-Tests gesetzt. Dieser soll zunächst fehlschlagen, soll heißen der Bug ist wirklich da und er ist auch wirklich verstanden. Erst dann wird Code hinzugefügt bis der Test "grün" wird.

Dabei habe ich die Oracle-Persistenz durch eine gemockte InMemory-Implementierung ersetzt. Der Insert wird dabei künstlich so verzögert dass ein konkurrierender Lesezugriff sicher dazwischenkommt. Wie man sieht etwas schwieriger als synchronen Code zu testen aber auch nicht viel schwieriger, oder?

[Test, Description("During atomic operations like insert/delete a concurrent read should be delayed/synchronized")]
public void Insert_should_lock_reading()
{
    var mock = new Mock<IGaps>();
    var backing = new InMemoryGaps();
    mock.Setup( gaps => gaps.Insert(It.IsAny<Gap>()) ).
         Callback( () => Thread.Sleep(delay) ).
         Returns<Gap>( backing.Insert );
    mock.Setup(gaps => gaps.FindById(It.IsAny<long>())).
         Returns<long>( backing.FindById );

    var sut = new GapsRemoteFacade(mock.Object);

    var expected = Gap.Copy(1);
    Task<Gap>.Factory.StartNew(() => sut.Insert(expected));

    var actual = sut.FindById(expected.Id);
    actual.Should().Be(expected, 
       "Atomic insert should lock concurrent read");
}

Ein Kollege hat mich darauf hingewiesen dass hier noch etwas fehlt:
Mit dem Test wird der Bug zunächst nur reproduziert, d.h. der Test soll zunächst fehlschlagen und tut das auch: actual ist null und nicht expected weil der sut.Insert(...) nicht mit konkurrierenden Lesezugriffen sut.FindById(...) synchronisiert ist. Ich zeige noch wie ich ihn "grün" gemacht habe, zum Einen der Vollständigkeit halber und zum Anderen weil es ein schönes Beispiel ist wie sich kleine Aspekte mit generischen Lambdas viel kompakter und eleganter trennen lassen als mit separaten Abstraktionen. Hier lautet der Aspekt "Synchronisierung" was man in Java einfach mit synchronized oder in .NET mit lock lösen könnte. Da hier viel mehr gelesen als geschrieben wird ist ReaderWriterLockSlim hier aber besser geeignet. Das typische Pattern für Lesezugriffe ist

private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();
...
 _lock.EnterReadLock();
try
{
    // do some reading
}
finally
{
    _lock.ExitReadLock();
}

Um den Code wartbar zu gestalten wollen wir vermeiden, dieses Pattern in jeder Methode zu wiederholen sondern die Synchronisierung separat halten. Ohne lambda expressions würde man hier wahrscheinlich entweder das Decorator-Pattern (d.h. eine weitere Subklasse SynchronizedGaps die alle Aufrufe wrappt) oder die Komposition mit einem Strategy-Pattern wählen (d.h. ein extra Interface nur für die Synchronisierung) was in aber genauso viel zusätzlich zu wartendem Code wie hier mit dem Lock resultieren würde. Kompakter geht es mit Lambdas indem man Methoden schreibt welche nur den Wrapping-Code zur Synchronisierung enthalten den auszuführenden inneren Code-Block jedoch als Argument:

public Gap Insert(Gap gap)
{
    return WithWriteLock(() =>
        {
            var newGap = _remoteGaps.Insert(gap);
            _gaps.Insert(newGap);
            return newGap;
        });
}

public Gap FindById(long id)
{
    return WithReadLock(() => _gaps.FindById(id));
}

private TResult WithReadLock<TResult>(Func<TResult> func)
{
    _lock.EnterReadLock();
    try
    {
        return func();
    }
    finally
    {
        _lock.ExitReadLock();
    }
}

private void WithWriteLock(Action action)
{
    _lock.EnterWriteLock();
    try
    {
        action();
    }
    finally
    {
        _lock.ExitWriteLock();
    }
}

Warum ist dieses Pattern schlanker? Weil man nur je eine Implementierung pro Methoden-Signatur braucht. Bevorzugt man gemäß Clean Code wenige Argumente lohnt sich die Einsparung eigentlich immer. Als Faustregel lässt sich dieses Pattern oft als schlanke Alternative zu einem Decorator verwenden. Typisch sind z.B. das Dekorieren einer Implementierung mit einem bestimmten Exception Handling, ein gewrappter CircuitBreaker oder dekorierte TimeOut-Logik. Mehr dazu im nächsten Post.