Проверка проекта Microsoft Orleans с помощью PVS-Studio

Введение

Всем доброго времени суток.

Вначале маленький Disclaimer для сомневающихся: да, за этот пост я, возможно, получу лицензию на PVS-Studio для проверки открытого проекта Microsoft Orleans. А может и не получу, как фишка ляжет-с. Нет, с компанией "СиПроВер" я напрямую никак не связан и написал этот пост по своей инициативе.

А теперь перейдем к сути.

PVS-Studio 6.0, как заявляет официальный сайт компании, это статический анализатор кода, ориентированный на простоту использования и поиск ошибок на этапе написания кода.

И относительно недавно, компания зарелизила версию, поддерживающую проверку C# проектов. Чем мы собственно и будем проверять проект Microsoft Orleans.

Кстати, команда PVS-Studio тоже проверяла проект Orleans на предмет выявленных ошибок, но я их немного опередил и они любезно предоставили мне свою КДПВ ("картинку для привлечения внимания") с неизменно радующим единорогом.

---

Что такое проект Orleans, Виртуальные актеры и в чем их профит.

Microsoft Orleans это framework, который построен на концепции виртуальных актеров. Терминология Orleans немного отличается от других подобных фреймворков (Akka.Net, Service Fabric, Erlang Actors): Актеры называются Grains (ака зерна), а сервера, участвующие в кластере — Silo.

Преимущество виртуальных актеров в том, что в любой момент ваш код может получить у Orleans Runtime прокси для обращения к конкретному Grain по его интерфейсу и Id. При вызове методов прокси посылается сообщение кластеру из нескольких серверов, где оно будет доставленому Grain-у с заданным Id. Runtime гарантирует, что такой Grain будет создан в единственном экземпляре на одном из серверов и последующие вызовы будут доставлены ему. Runtime также автоматически удаляет из памяти (деактивирует) Grain-ы, которые не получали вызовов заданное время, таким образом постоянно собирая "мусор". Если сервер на котором находилось ранее ваше "зерно", ушел в оффлайн — Runtime быстро поднимет инстанс на другом и вы ничего не заметите, кроме небольшой задержки. Если вызов должен прийти на тот же самый сервер — Runtime это оптимизирует и вызов будет локальным.

Собственно, профит виртуальных актеров в том, что это все очень легко масштабируется в облаках:
Silo пингуют друг друга и определяют когда кто-то недоступен, перераспределяют зерна "павшего камрада" между собой и все такое. Кластер радостно живет и доступен, пока есть хотя бы одно активное Silo, и при подключении новых участников — они получат свой диапазон "зерен" и начнут активно обрабатывать запросы. А свой код вы пишите, используя привычный ООП-подход. Получается такой, как бы "Distributed C#/.NET".

Еще сам рантайм дает гарантии что при выполнении метода на зерне, никакой другой вызов не на то же самое зерно не прийдет, т.е. рантайм гарантирует single-threaded выполнение вашего кода, что позволяет меньше думать о всяких не-thread-safe ситуациях и больше сосредоточиться на написании полезной бизнес-логики.

Вообще, там еще много других интересных вещей, таких как заменяемые провайдеры хранилища, messaging, pub-sub и другие полезные вещи для разработки приложений под облачные (или распределенные on-prem) платформы.
Подробней с проектом можно ознакомиться здесь — Microsoft Orleans@GitHub. Microsoft Orleans неплохо протестирован и в масштабных проектах — этот фреймворк используется на бакенде игр Halo 4 и Halo 5 — собирает информацию о всех играх, аггрегирует статистику и т.д.

---

Почему захотелось проверить Orleans

Ну во-первых, мы сами используем Orleans для создания нашей облачной платформы в Drawboard и хочется полагаться на удобный и надежный фреймворк.

Во-вторых, концепции и гарантии, заложенные в Orleans, довольно нетривиальны и от их реализации зависит качество работы кластера.

В-третьих — было просто интересно попробовать PVS-Studio с С# — команда продукта пишет отличные статьи про проверку С++ проектов, а вот C# был как-то обделен вниманием до последнего времени.

---

Результаты проверки

Проект Microsoft Orleans развивается очень динамично и, очевидно, разовая проверка хоть и поможет найти сомнительные места — в длинной перспективе на общее качество повлияет не сильно.

Итак, по состоянию на момент мержа PR #1288 4/02/2016 2:43:26 PM, Commit hash: 7c1e35466fde08fcf1c2caf64fa304d25e60e045 PVS-Studio (версия 6.01.15638.1) выдала:

• 18 High-severity предупреждений
• 7 Medium-severity предупреждений
• 58 Low-severity предупреждений

Трудно сказать хорошо это или плохо, потому как это нельзя сравнивать с другими проектами, проверенными PVS-Studio — разная сложность, количество кода, компетентность разработчиков и множество других факторов.
Но в целом — выглядит не самым худшим результатом, количество подозрительных мест не выражается трех или четырехзначным числом — все просмотреть и обработать можно за день.

Посмотрим, найдет ли PVS-Studio что-то серьезное.

И еще — вот таблица, которая показывает разделение ошибок между основным кодом (Runtime и вспомогательные проекты) и кодом в тестах:

Severity	Total	Runtime	Tests
High	18	12 *	6
Medium	7	4	3
Low	58	13	45

* — 4 предупреждения на неправильное использование Replace в одном методе, 3 предупреждения в малоиспользуемом коде. Итого ~5 на которые стоит обратить внимание.

Я пройдусь, в основном, по ошибкам в Runtime, т.к. ошибки в тестах обычно менее критичны. Хотя они могут создать иллюзию, что все ОК, но на самом деле в реальности все будет не так радужно ...

Критичные предупреждения, обнаруженные в проекте

Начнем с самой интересной и оказавшейся довольно серьезной. Разработчики проекта, оказывается, тоже столкнулись с проблемой, восходящей корнями к этому багу

(это из Gitter-чата проекта о найденной проблеме):
...an embarrassing bug in SanitizeTableProperty that IIRC puzzled us recently.
...were sure the sanitization worked.

PVS-Studio выдала вот такое предупреждение:

V3010 The return value of function 'Replace' is required to be utilized. AzureStorageUtils.cs 278,279,280,281

И по указанном адресу находился (баг уже исправлен) метод:

public static string SanitizeTableProperty(string key)
{
    // Remove any characters that can't be used in Azure PartitionKey or RowKey values
    key.Replace('/', '_');  // Forward slash
    key.Replace('\', '_'); // Backslash
    key.Replace('#', '_');  // Pound sign
    key.Replace('?', '_');  // Question mark
    if (key.Length >= 1024)
        throw new ArgumentException(string.Format("Key length {0} is too long to be an Azure table key. Key={1}", key.Length, key));
    return key;
}

Довольно классный баг, прямо snake oil (… и тут медленно пришло прозрение что этот баг скорей всего вылезал и в нашем коде пару раз....).
Параметр key типа string, к тому же передается в метод. Строки в .NET immutable, так что сколько бы раз мы не вызывали key.Replace — значение не изменится.

Следующая неплохая находка -

V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new InconsistentStateException(FOO). MemoryStorageGrain.cs 129

if (currentETag != null)
{
    string error = string.Format("Etag mismatch during {0} for grain {1}: Expected = {2} Received = null", operation, grainStoreKey, currentETag.ToString());
    logger.Warn(0, error);
    new InconsistentStateException(error);
}

Новое исключение создается, но не выбрасывается. Т.е. если при записи присылается Etag=null и это первая запись, ошибки не происходит. Чуть ниже в том же методе другое исключение все же выбрасывается — т.е. у нас тут пропущенная проблема.

Еще одно предупреждение:

V3005 The 'jsonSettings' variable is assigned to itself. AzureTableStorage.cs 104

При чтении конфигурации настройки сериализации в переменной jsonSettings инициализируются 2 раза:

if (useJsonFormat)
{
    jsonSettings = jsonSettings = OrleansJsonSerializer.SerializerSettings;
}

Не сильно критичная проблема, по мне так выглядит как результат рефакторинга — в "прошлой жизни" одна из этих переменных скорей всего называлась по-другому. Потом отдельную переменную для дефолтных настроек убрали, а присвоение осталось.

Следующее (false alarm в нашем случае):

**V3022 Expression 'USE_DEBUG_CONTEXT_PARAMS && arguments != null && arguments.Length > 0' is always false. GrainReference.cs 480***

[NonSerialized] private const bool USE_DEBUG_CONTEXT_PARAMS = false;
...
if (USE_DEBUG_CONTEXT_PARAMS && arguments != null && arguments.Length > 0)
{
    ...
}

Отладочный флаг, компилятор оптимизирует эту ветку if. Не очень критично, из контекста понятно что произойдет. Но проверять такие места все же полезно — чтобы убедиться что важный и нужный код не будет "с-оптимизирован".

Это предупреждение найдено в малоиспользуемом коде, поэтому этот участок не вызывал особых проблем, но мог бы:

V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 3. Present: 2. Program.cs 169,183

WriteStatus(string.Format("**Calling DeleteGrain({0}, {1}, {2})", silo, grainId));

WriteStatus(string.Format("**Calling LookupGrain({0}, {1}, {2})", silo, grainId));

Может показаться, что это не такая уж и существенная ошибка, ну логгер, ну что-то не запишет. Но нет. На самом деле тут вылетит —

FormatException: Index (zero based) must be greater than or equal to zero and less than the size of the argument list.

А следующее предупреждение может сигнализировать о реальных проблемах в логике кода, в Orleans это, к счастью, только логгирование -

V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. Interner.cs 251

private void InternCacheCleanupTimerCallback(object state)
{
...
   long numRemoved = numEntries - internCache.Count;
   if (numRemoved>0)
       if (logger.IsVerbose) logger.Verbose(ErrorCode.Runtime_Error_100296, "Removed {0} / {1} unused {2} entries in {3}", numRemoved, numEntries, internCacheName, clock.Elapsed);
   else
       if (logger.IsVerbose2) logger.Verbose2(ErrorCode.Runtime_Error_100296, "Removed {0} / {1} unused {2} entries in {3}", numRemoved, numEntries, internCacheName, clock.Elapsed);
}

Это стилистически сложно оформленный код — не очень понятно, что хотели сделать. Разобраться конечно можно, но такой стиль не только ломает глаза читающему код, но и открывает возможности для всяких неприятных багов, подобных сюрпризу от Apple.

Apple certificate check bug:
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
goto fail;
goto fail;

Для того чтобы такие вещи не появлялись — есть StyleCop и много других методов "принуждения к правильному стилю". И их тоже полезно использовать.

Еще одно предупреждение, тоже незначительное в данном случае, просто избыточная проверка. Но если проверка вычислительно "дорогая" — стоит от нее избавиться.

V3053 An excessive expression. Examine the substrings '/bootstrap' and '/boot'. ClientGenerator.cs 310

else if (arg.StartsWith("/bootstrap") || arg.StartsWith("/boot")){...}

На этом критические ошибки, которые достойны внимания, заканчиваются и впереди еще немного Medium и побольше Low.

Найденные ошибки средней тяжести

Первым идет пример предупреждения, который, может быть как сигналом проблемы, так и поводом для холивара.

V3054 Potentially unsafe double-checked locking. Use volatile variable(s) or synchronization primitives to avoid this. StreamImpl.cs 142, 144

Пример кода немного порезан, оставлены только основные участки с некоторыми комментариями.

private readonly object initLock; // need the lock since the same code runs in the provider on the
...
internal IAsyncBatchObserver<T> GetProducerInterface()
{
->>  // not so сanonical double-checked locking, effectively doing the same,
    if (producerInterface != null) return producerInterface;
    lock (initLock)
    {
        if (producerInterface != null) 
            return producerInterface;

        if (provider == null)
            provider = GetStreamProvider();

        producerInterface = provider.GetProducerInterface<T>(this);
    }
    return producerInterface;
}

internal IInternalAsyncObservable<T> GetConsumerInterface()
{
->> // Canonical double-checked locking
    if (consumerInterface == null)
    {
        lock (initLock)
        {
            if (consumerInterface == null)
            {
                if (provider == null)
                    provider = GetStreamProvider();

                consumerInterface = provider.GetConsumerInterface<T>(this);
            }
        }
    }
    return consumerInterface;
}

Тут у нас два примера применения Double-checked locking паттерна. Чак Норрис мира .NET (aka Jon Skeet) в статье Implementing the Singleton Pattern in C# приводит более изящную и надежную реализацию, если нужен синглтон.

Я еще хотел написать что:

Но вот лично у меня этот код вызывает еще одно сомнение: во всех статьях и примерах этот паттерн всегда использует lock на static объект, и вот я не очень уверен, что его можно применять на non-static локах с гарантированно надежным результатом…

Но, после общения с разработчиками и чтения вот этой статьи Sayonara volatile by Joe Duffy, соглашусь, что т.к. в нашем случае это не синглтон, допустимо использование не-статического поля. И без volatile.

Вообще, изучая эту ошибку, я открыл для себя такую "банку червей", что уже даже не знаю — проблема ли это в коде или конкретно с этой диагностикой у PVS-Studio.

Но сам факт, что инструмент может ловить такие паттерны вообще — это, имхо, здорово. И надеюсь, что в будущем мы увидим больше изощренных предупреждений, хороших и разных.

Переходим к еще одному неплохому примеру, баг, который очень плохо ловится человеком, но легко — анализатором:

V3022 Expression 'n1 == 0 && n1 != 0' is always false. Unsigned type value is always >= 0. Probably the '||' operator should be used here. UniqueKey.cs 113

private static UniqueKey NewKey(ulong n0, ulong n1, Category category, long typeData, string keyExt)
{
    // in the string representation of a key, we grab the least significant half of n1.
    // therefore, if n0 is non-zero and n1 is 0, then the string representation will always be
    // 0x0 and not useful for identification of the grain.
    if (n1 == 0 && n1 != 0)
        throw new ArgumentException("n0 cannot be zero unless n1 is non-zero.", "n0");

Тут надо было проверить n0!=0, как и написано в комментарии к этому коду, а в текущей реализации проверка всегда false. Опять же, хороший Coding-style мог бы помочь в данном случае — если бы переменные не назывались n0 и n1, а например firstHalf и secondhHalf — ошибка бы бросалась в глаза более явно. Сравните:

if (firstHalf == 0 && secondHalf != 0)
    vs
if (secondHalf == 0 && secondHalf != 0)

Предупреждение об одинаковом коде в двух разных методах —

V3013 It is odd that the body of 'IncrementMetric' function is fully equivalent to the body of 'DecrementMetric' function (1079, line 1095). TraceLogger.cs 1079

Spaced Copy-Paste, между этими 2-мя методами есть еще и правильная реализация
декремента, который уменьшает метрику.

public override void IncrementMetric(string name, double value)
{
    foreach (var tc in TelemetryConsumers.OfType<IMetricTelemetryConsumer>())
    {
->>     tc.IncrementMetric(name, value);
    }
}
...
public override void DecrementMetric(string name, double value)
{
    foreach (var  tc in TelemetryConsumers.OfType<IMetricTelemetryConsumer>())
    {
->>     tc.IncrementMetric(name, value);
    }
}

То же самое, но в тестах:

V3013 It is odd that the body of 'StartTimer' function is fully equivalent to the body of 'StopTimer' function (183, line 188). TimerOrleansTest.cs 183

public Task StartTimer(string timerName)
{
    if (persistant) return persistantGrain.StartTimer(timerName);
    else return grain.StartTimer(timerName);
}
public Task StopTimer(string timerName)
{
    if (persistant) return persistantGrain.StartTimer(timerName);
    else return grain.StartTimer(timerName);
}

Этот привет от Ctrl+C, Ctrl+V в тестах может иметь и худшие последствия — тесты будут false-positive.

Сложный код в тестах может выдавать одну ошибку на 10 или 100 запусков, тогда такой тест еще и вызывает раздражение :

V3032 Waiting on this expression is unreliable, as compiler may optimize some of the variables. Use volatile variable(s) or synchronization primitives to avoid this. LoggerTest.cs 468

// Wait until the BulkMessageInterval time interval expires before wring the final log message - should cause bulk message flush
while (stopwatch.Elapsed <= TraceLogger.BulkMessageInterval)
{
    Thread.Sleep(10);
}

Честно говоря, я сам не очень хорошо понимаю, как и почему цикл здесь может превратиться в бесконечный, но пример в описании этого предупреждения на сайте PVS-Studio более понятный V3032.
Не очень большая проблема, т.к. код в тестах, но даже в теории — произвольно падающие тесты или надолго зависающие — не самая приятная штука.

И опять странный код в тестах -

V3051 An excessive type check. The object is already of the 'Exception' type. PersistenceGrainTests.cs 178

catch (AggregateException ae)
{
    exceptionThrown = true;
    Exception e = ae.GetBaseException();
->> if (e is Exception)
    {
        // Expected error
    }
    else
    {
        throw e;
    }
}

Исключение никогда не будет выкинуто повторно, потому что в ветку else код никогда не попадет. Критичность зависит от контекста, в тестах может и не так страшно, а в другом коде — чистой воды бага

Неопасные (low-severity) ошибки

Довольно много замечаний PVS-Studio обнаружила в тестах, но просмотрев большинство из них, можно сделать вывод, что критических или high-impact проблем среди них нет. Просто комплексно протестировать такой продукт довольно сложно и часто приходится идти на разные ухищрения, чтобы вызвать определенное поведение системы.

Например вот:

V3008 The 'promise' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 91, 84. TestInternalGrains ErrorGrain.cs 91

// the grain method returns OK, but leaves some unobserved promise

-->>Task<long> promise = Task<long>.Factory.StartNew(() =>
{
    if (!doThrow)
        return 0;
    logger.Info("About to throw 1.");
    throw new ArgumentException("ErrorGrain left Immideate Unobserved Error 1.");
});
-->>promise = null;
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
GC.WaitForPendingFinalizers();
return Task.FromResult(11);

Проверяется поведение кода с таском, который собран GC.

Из того, на что еще стоит обратить некоторое внимание (эти ошибки могут привести к некорректным ожиданиям при тестах):

V3013 It is odd that the body of 'ProduceSequentialSeries' function is fully equivalent to the body of 'ProduceParallelSeries' function (618, line 625). StreamingGrain.cs 618

public override async Task ProduceSequentialSeries(int count)
{
    await base.ProduceParallelSeries(count);
    State.Producers = _producers;
    await WriteStateAsync();
}

public override async Task ProduceParallelSeries(int count)
{
    await base.ProduceParallelSeries(count);
    State.Producers = _producers;
    await WriteStateAsync();
}

У специального класса для тестирования методы, предназначенные для генерации последовательных и параллельных серий событий, всегда генерируют параллельные. Может это и ОК, но тогда семантика внутри теста вводит в заблуждение.

V3013 It is odd that the body of 'TestInitialize' function is fully equivalent to the body of 'TestCleanup' function (44, line 52). ConfigTests.cs 44

[TestInitialize]
public void TestInitialize()
{
    TraceLogger.UnInitialize();
    GrainClient.Uninitialize();
    GrainClient.TestOnlyNoConnect = false;
}

[TestCleanup]
public void TestCleanup()
{
    TraceLogger.UnInitialize();
    GrainClient.Uninitialize();
    GrainClient.TestOnlyNoConnect = false;
}

В тестах конфигурации при инициализации и при завершении зачем-то дважды проводится ДЕ-инициация примитивов.

V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. RequestContextTest.cs 87,97,111,155,181

if(msg.RequestContextData != null) foreach (var kvp in msg.RequestContextData)
{
    headers.Add(kvp.Key, kvp.Value);
};

И все-таки, я большой сторонник строгих Coding-style правил. Ну или хотя бы автоматического принуждения к стилю, путем StyleCop, Resharper или CodeFormatter. Это ничего не стоит, но позволяет не ломать глаза.

V3052 The original exception object 'ae' was swallowed. Stack of original exception could be lost. GrainReferenceCastTests.cs 212

catch (AggregateException ae)
{
    Exception ex = ae.InnerException;
    while (ex is AggregateException) ex = ex.InnerException;
    throw ex;
}

Теряем стек AggregatedException. Я понимаю, что для случаев с другими исключениями это плохо, но мы в нашем проекте точно так же "раскручиваем" AggregatedException. Я бы записал это в false-positive если в catch секции именно AggregatedException.

Оценка критичности найденых багов разработчиками проекта.

Из всех отправленных отчетов об ошибках, спустя 3 часа 5 отмечены как bug, 1 критический с Replace исправлен. Довольно неплохой улов для 20 минут статического анализа в автоматическом режиме.

Почему Code analysis это не очередной favour of the month.

---

Проект Orleans развивается очень интенсивно, частота коммитов очень высокая, Gihub это подтверждает — Orleans-contributors. За время написания статьи, я проверил проект несколько раз, сохраняя логи анализа. В том числе разными версиями PVS Studio (6.0 и 6.01).

Вот сравнение разных логов:

Priority	28 Янв	2 Фев	4 Фев v6.01	Comments
High	19	21	18	За два дня -3 критичные ошибки — что хорошо. Но, например, 2 из них были аналогами V3025 описанной выше. Можно было бы даже не чекинить такой код.
Medium	4	4	7	За эти же 2 дня +3 предупреждения средней тяжести.
Low	52	46	58	И +12 новых участков неаккуратного кода.

Как видно из таблицы, критические ошибки появляются и исчезают за очень короткое время. Куда как удобней отлавливать их в результате анализа сразу после билда, а не после часов дебага или падения продакшена — экономит время и нервы программиста. Это, конечно, банальность из серии "Лучше быть богатым и здоровым, чем бедным и больным", но если еще есть программисты, которые не используют хоть какой-нибудь доступный статический анализатор кода — "You're doing it wrong" ...

В плане удобства использования — интеграция у PVS-Studio с Visual Studio 2015 довольно простая — в контекстном меню Solution Explorer на С# файле, проекте, или корневом solution есть пункт Analyze with PVS-Studio с хорошо заметной зеленой иконкой. Довольно легко найти. Если только в этом контекстном меню нету еще 100500 пунктов от других расширений. Было бы интересно увидеть какой-нибудь вспомогательный nuget пакет, который бы умел легко запускать анализ из командной строки, чтобы можно было открыть Package Manger Console или build.cmd и сказать PVS-Solution или PVS-Project Orleans, ну и в будущем это все как-то встроилось в xproj механизмы нового CoreCLR.

Хотя, для быстрого запуска хватает и Ctrl+Q, PVS ... Down, Down, Down, Down, Enter :).

Заключение

---

Современные реалии разработки задают очень высокую планку в скорости развития — все стараются бежать вперед очень быстро, чтобы хотя бы оставаться на уровне конкурентов в отрасли. Естественно, что с такой скоростью развития можно что-то упустить или написать небрежно. Обширный набор тестов хорошо помогает в определении проблемных мест и регрессий, но это тоже не панацея — в тестах бывают ошибки, да и создание такого набора, как и поддержание его в актуальном состоянии требует времени и усилий.

Любой статистический анализатор, будь то предупреждения Visual Studio, анализатор Resharper'а или PVS-Studio, это еще один инструмент в коллекции разработчика и помощник при обнаружении потенциальных проблем с кодом. Используйте хотя бы то, что доступно бесплатно. Например, на всех наших проектах по умолчанию включен режим Treat Warnings as Errors, что помогает писать код чуть более дисциплинированно. Это ничего не стоит программисту, но может сберечь его от выстрелов себе в ногу. Современные анализаторы использовать очень легко, в большинстве случаев всё отлично работает "из коробки" и единственное, что может хоть как-то оправдать не-использование анализаторов, это вопрос цены.

NB: Автор благодарит команду PVS-Studio за предоставленную временную лицензию для тестирования.

Всем прочитавшим — Happy and bug-free programming!

Автор: centur

Источник