вторник, 8 марта 2016 г.

ErrorProne.NET. Часть 2

В прошлый раз мы остановились на проблемах с форматированием, а сегодня пришло время заняться исключениями.

ErrorProne.NET унаследовал многие возможности из моего другого проекта – ExceptionAnalyzer, но в несколько измененном виде. Вот какие правила он содержит:

  • ERP021 – Incorrect exception propagation: «неправильный» проброс исключения с помощью throw ex; вместо throw;
  • ERP022 – Unobserved exception in generic exception handler: когда блок catch, который перехватывает все исключения возвращает управления без попытки «наблюдения» перехваченного исключения.
  • ERP023 – Suspicious exception handling: only Message property was observed: когда в обобщенном блоке catch идет обращение лишь к свойству ex.Message исключения.

Теперь обо всех этих исключениях более подробно.

Incorrect exception propagation

На этой проблеме нет смысла останавливаться слишком долго. Если вы проходили хоть одно собеседование в своей жизни или работали в команде из более чем пары человек, то наверняка знаете, в чем разница между throw ex; и throw;:

try { throw new Exception(); }
catch (Exception
ex)
{
   
throw
ex;
   
//    ~~
    //    Incorrect exception propagation. Use throw; instead
}

Тут никаких сюрпризов. throw ex; «сломает» стек вызовов оригинального исключения в результате чего последующий анализ логов и понимание того, что же пошло не так, будет затруднительным. Поскольку эта известная проблема, то подобные ошибки встречаются на практике довольно редко. Но несмотря на относительную это, я нашел один пример сомнительного проброса исключений в коде Розлина: CompilationWithAnalyzer.cs:602.

Unobserved exception in generic exception handler

Поскольку идея этой тулы в поиске максимально подозрительных мест, большинство правил, связанных с обработкой исключений являются простыми, конкретными и не слишком назойливыми. Так, например, как бы ни хотелось пуристам запретить все блоки catch(Exception) в их коде, на практике сделать это будет довольно сложно ввиду их распространенности. Но, ИМХО, можно и нужно предупреждать, если такие блоки возвращают управления даже не удосужившись проверить, что же за исключение они поймали.

Поэтому, следующий код приводит к выдаче предупреждения:

try { throw new Exception(); }
catch (Exception
e)
{
   
if (e is AggregateException) return
;
   
//                           ~~~~~~~
    // Exit point 'return;' swallows an exception.
    throw;
}

Любое обращение к переменной e в блоке catch (типа e.Message, ProcessExcpetion(e), etc) «деактивирует» это правило. Я почему-то считал, что срабатывать это правило должно относительно нечасто, и был крайне удивлен, что на коде Розлина, оно срабатывает более 60 (!) раз. Вот лишь несколько примеров, для затравки.

Да, это же правило будет срабатывать только на блоки catch {}, catch(System.Exception) и catch(System.AggregateException). Для более специфических исключений, вполне нормально возвращать управление без обращения к объекту исключения.

Suspicious exception handling: only Message property was observed

Что может быть хуже проглатывания исключений? Записывание не всей информации об исключении! Каждый раз, когда вы пишите лишь ex.Message в лог файл, всякие непонятные высокие силы делают так, чтобы ваш код грохнулся в продакшне с TypeLoadException и вас подняли в 2 часа ночи для того, чтобы выяснить, что же пошло не так с чудесно проверенной на локальном компе билдом. Чтобы вы с просоня смотрели в лог и долго думали над волшебными записями в логе типа: «Exception has been thrown by the target of an invocation.», «The type initializer for 'MyAwesomeType' threw an exception.» или «One or more errors occurred.» и гадали, “А что же, блин, произошло!».

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

class WillThrow
{
   
public
WillThrow()
    {
       
throw new Exception("Oops!"
);
    }
}

public static T Create<T>() where T : new
()
{
   
return new T
();
}

public void
Sample()
{
   
try { Create<WillThrow
>(); }
   
// Warn for ex.Message: Only ex.Message was observed in the exception block!
    catch (Exception
ex)
    {
       
// ex.Message:
        // Exception has been thrown by the target of an invocation.
        Console.WriteLine(ex.
Message);
       
//                ~~~~~~~~~~
        // Only ex.Message property was observed in exception block!
    }
}

Поскольку ограничение new() приводит к использованию Activator.CreateInstance, то любое исключение в конструкторе объекта, создаваемого с помощью обобщенных методов, типа Create<T>, приведет к обарачиванию исходного исключения в TargetInvocationExcpetion. Такое легко пропустить во время ревью, но гораздо проще заворачивать любой код, который пишет лишь сообщение исключения, и не выводит стек-трейса + внутренних искючений.

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

Ссылки

Комментариев нет:

Отправить комментарий