MassTransit — Open Source платформа распределённых приложений для .NET. В этой статье мы расскажем о проблемных местах в коде проекта. С поиском таких мест нам поможет статический анализатор. Приятного чтения :)
Введение
Так как MassTransit предоставляет публичное API, качество кода такого проекта имеет особенно важную роль. Неожиданное поведение данного продукта может негативно сказаться не только на нём самом, но и на использующих его приложения. Это одна из причин, по которой мы решили проверить MassTransit. Для анализа был взят исходный код, соответствующий версии v8.0.15.
Не будем томить, приступим к разбору наиболее интересных фрагментов кода.
Подозрительное условие
Issue 1
Предупреждение PVS-Studio: V3001 There are identical sub-expressions '!context.ReceiveContext.IsDelivered' to the left and to the right of the '&&' operator. OutboxMessagePipe.cs 57
Обратите внимание на условие if. Выглядит странно, ведь проверяется значение одного и того же свойства. Очевидно, что подобная проверка не имеет смысла. Однако если посмотреть свойства, к которым можно обратиться у объекта context.ReceiveContext, то можно найти IsFaulted. Возможно, именно оно должно фигурировать в одном из случаев.
Issue 2, 3
десь PVS-Studio выдаёт сразу два предупреждения:
- V3022 Expression 'string.IsNullOrEmpty(binPath)' is always true. AssemblyFinder.cs 23;
- V3142 Unreachable code detected. It is possible that an error is present. AssemblyFinder.cs 26.
Анализатор сообщает нам, что в данном коде обнаружено всегда истинное условие, а также недостижимый код. Давайте разберёмся, как же так получилось.
В переменную binPat присваивается пустая строка. Сразу за присваиванием проверяется, что значение этой переменной равно null или пустой строке. Результат такой проверки — всегда истина. Получается, что выполнение метода всегда заканчивается на этом условии, так как в блоке then возвращается значение.
Тяжело сказать, является это ошибкой или такое поведение реализовано намерено. В любом случае код выглядит подозрительно, так как теряется немалая часть логики метода.
Issue 4
Предупреждение PVS-Studio: V3022 Expression 'reader.TokenType == JsonTokenType.Null' is always false. DoubleActivity_Specs.cs 55
Рассмотрим первое условие метода Read. В нём производится проверка значения reader.TokenType. Если оно не соответствует JsonTokenType.StartObject, то будет выброшено исключение. Из этого следует, что после условия и до следующего вызова метода Read, reader.TokenType будет иметь значение JsonTokenType.StartObject. Получается, что в блок then второго if поток выполнения не зайдёт никогда. Возможно, после первого if был пропущен вызов Read.
Потенциальное обращение по отрицательному индексу
Issue 5
Предупреждение PVS-Studio: V3106 Possible negative index value. The value of 'paramCount' index could reach -1. ExpressionCompiler.cs 555
Обратите внимание на переменную paramCount. Её значение — размер массива, переданного в качестве аргумента, уменьшенный на 1. Дальше это значение используется в качестве индекса. Однако проверка перед использованием не гарантирует, что индекс не отрицательный. В случае обращения по индексу с отрицательным значением будет выброшено исключение.
Возможно, разработчики уверены в том, что closurePlusParamTypes всегда содержит хотя бы один элемент. Может быть, в данный момент это действительно так. Но нет гарантий того, что в будущем в данный метод не будет передана пустая коллекция.
Issue 6
Предупреждение PVS-Studio: V3106 Possible negative index value. The value of 'index' index could reach -1. ExpressionCompiler.cs 1977
Анализатор указывает на потенциальное обращение по отрицательному индексу. Переменная index содержит значение, использующееся в качестве индекса. Оно будет получено в результате выполнения метода GetLabelOrInvokeIndex. Рассмотрим определение этого метода:
Здесь мы видим, что один из вариантов возвращаемого значения — -1. Нетрудно догадаться, что такой сценарий не совсем желателен, так как будет выброшено исключение.
Стоит отметить, что при работе с возвращаемым значением GetLabelOrInvokeIndex в ряде других мест присутствует проверка на то, что оно не равно -1. Одна их них:
Неиспользуемый параметр
Issue 7
Предупреждение PVS-Studio: V3117 Constructor parameter 'initialize' is not used. BusHostInfo.cs 17
Параметр одной из перегрузок конструктора класса BusHostInfo не был использован. Стоит отметить, что у данного класса есть публичный конструктор без параметров, который вообще ничего не делает. В итоге получается следующая ситуация: если использовать конструктор с параметром, то вне зависимости от его значения инициализация производится. Если же воспользоваться конструктором, который не принимает параметров, то инициализация отсутствует. Такое поведение выглядит весьма странно.
Возможно, раньше параметр влиял на логику работы, однако перестал после рефакторинга. Разработчик мог намерено не удалять неиспользуемый параметр, чтобы избежать ошибок у пользователей. Ошибки могли возникнуть из-за того, что класс BusHostInfo публичный, как и рассматриваемый конструктор. Это значит, что он доступен пользователям напрямую. Если поменять сигнатуру метода (убрать параметр), то в местах вызова данного конструктора возникнут ошибки из-за отсутствия нужной перегрузки. Если данное предположение верно, то хотелось бы видеть комментарий, поясняющий этот момент.
Заключение
Можно сказать, что код проекта оказался чистым. Во многом это заслуга разработчиков, однако стоит учесть, что они уже использовали другой статический анализатор. Это стало понятно, исходя из комментариев для подавления его срабатываний. Даже при этом условии удалось найти ряд подозрительных мест. Возможно, анализатором пользовались ранее, а сейчас перестали.
В любом случае, надеюсь, что эта статья поможет разработчикам найти и исправить ошибки, чтобы повысить качество и чистоту кода проекта :).
Если у вас появилось желание проверить исходный код интересующего проекта, то вы можете бесплатно попробовать PVS-Studio. Удачного использования!
Еще больше интересных статей вы можете найти у нас в блоге!
