Blog Kokosa

Przygody z deadlockiem (czy jak kto woli – zakleszczeniem) w bibliotece ClrMd ciąg dalszy. W poprzedniej części obszedłem problem po stronie aplikacji, jednak chciałbym przyczynić się do poprawy życia ludzkości i naprawić ten problem w samej bibliotece. Widzę tu trzy wyjścia.

1. Bardziej świadome obsługa Wait()

Jeśli już chcemy naszą bibliotekę uodpornić na problemy z użyciem Wait(), powinniśmy pamiętać o metodzie Task.ConfigureAwait.

Jej jedyny argument o nazwie continueOnCapturedContext decyduje, czy kontynuacja po wykonaniu zadania ma wrócić do oryginalnego kontekstu, czy też nie. Twórcy bibliotek, którzy przecież nie wiedzą czy ten powrót do kontekstu nie sprawia problemów (jak w DispatcherSynchronizationContext dla WPF), powinni jak mantrę zapamiętać to wywołanie.

"TL;DR version of the long answer: If you are writing a library where you don't know your consumer and don't need a synchronization context (which you shouldn't in a library I believe), you should always use ConfigureAwait(false). Otherwise, the consumers of your library may face deadlocks by consuming your asynchronous methods in a blocking fashion. This depends on the situation." [1]

Poprawka w tym wypadku jest prosta, w metodzie sync Task CopyStreamToFileAsync zamieniamy (kod przedstawiam w uproszczeniu):

output = new FileStream(fullDestPath, FileMode.CreateNew);
Task result = input.CopyToAsync(output);
await result;

na

output = new FileStream(fullDestPath, FileMode.CreateNew);
Task result = input.CopyToAsync(output);
await result.ConfigureAwait(continueOnCapturedContext: false);

I program magicznie zaczyna poprawnie działać również w pierwotnej, synchronicznej wersji wywołań! Wadą tego podejścia jest to, że sama klasa SymbolLocator ma sporo innych awaitów, które powinniśmy opatrzyć taką samą zmianą. A to już dość duża modyfikacja.

2. Świadoma kontrola felernego wywołania

Skoro namierzyłem jeden konkretny Wait, który sprawia problemy, rozsądne wydaje się skoncentrowanie tylko na nim:

protected override void CopyStreamToFile(Stream stream, string fullSrcPath, string fullDestPath, long size)
{
    CopyStreamToFileAsync(stream, fullSrcPath, fullDestPath, size).Wait();
}

I przepisanie wywołania na takie, które świadomie używa ThreadPool, opakowując asynca:

protected override void CopyStreamToFile(Stream stream, string fullSrcPath, string fullDestPath, long size)
{
    var task = Task.Run(async () => { await CopyStreamToFileAsync(stream, fullSrcPath, fullDestPath, size); });
    task.Wait();
}

Dzięki czemu nie musimy modyfikować metody CopyStreamToFileAsync , co wydaje się dość wygodne. Mała lokalna zmiana, na mały konkretny problem. Aczkowiel kod ten nie wygląda pięknie, jestem tego świadomy.

3. Podróż w poprawnym kierunku "async all the way"

Idąc w tym poprawnym kierunku, nie wywołujemy nigdzie Wait() w ramach biblioteki, ale metodę zamieniamy na asyncową aż do publicznego API. Zatem zamiast:

public ClrRuntime CreateRuntime()
{
    string dac = _dacLocation;
    if (dac != null && !File.Exists(dac))
        dac = null;

    if (dac == null)
        dac = _dataTarget.SymbolLocator.FindBinary(DacInfo);

    if (!File.Exists(dac))
        throw new FileNotFoundException(DacInfo.FileName);

    if (IntPtr.Size != (int)_dataTarget.DataReader.GetPointerSize())
        throw new InvalidOperationException("Mismatched architecture between this process and the dac.");

    return ConstructRuntime(dac);
}

tworzymy:

public async Task<ClrRuntime> CreateRuntimeAsync()
{
    string dac = _dacLocation;
    if (dac != null && !File.Exists(dac))
        dac = null;

    if (dac == null)
        dac = await _dataTarget.SymbolLocator.FindBinaryAsync(DacInfo);

    if (!File.Exists(dac))
        throw new FileNotFoundException(DacInfo.FileName);

    if (IntPtr.Size != (int)_dataTarget.DataReader.GetPointerSize())
        throw new InvalidOperationException("Mismatched architecture between this process and the dac.");

    return ConstructRuntime(dac);
}

Musimy zatem też zmienić "konsumpcję" nowego asynchronicznego API w programie:

member this.Load() = async {
    use target = DataTarget.LoadCrashDump(@"..\..\..\..\data\SampleConsoleApplication.exe.dmp")
    target.SymbolLocator.SymbolPath <- @"SRV*http://msdl.microsoft.com/download/symbols"
    target.SymbolLocator.SymbolCache <- @"c:\symbols"
    for version in target.ClrVersions do
        let dacInfo = version.DacInfo
        let! runtime = Async.AwaitTask (version.CreateRuntimeAsync())
        // ...
}

Wtedy zaś użycie w programie musi świadomie wziąć na siebie czekanie na odpowiedź:

dump.Load() |> Async.RunSynchronously

And the winner is...

Szczerze mówiąc, nie wiem które rozwiązanie jest lepsze. Podejście 1. wydaje się dość oczywistym poprawieniem IMHO niedopatrzenia autora biblioteki. Trzecie podejście wymagało by znacznie szerszych i bardziej przemyślanych zmian - czemu bowiem zmieniać na asynca akurat tylko tą metodę API? Pewnie trzeba by to zrobić wszędzie.

Zatem w swoich zapędach naprawy świata na razie wybrałem opcję 2. – najbardziej lokalną. Zobaczymy co się stanie. Żeby nie przedłużać już tego posta, w następnej części przygotuję Pull Requesta do repozytorium ClrMd.

1. Best practice to call ConfigureAwait for all server-side code