Try / Catch / Log / Rethrow - это Anti Pattern?

19

Я вижу несколько постов, где важность обработки исключений в центральном местоположении или на границе процесса подчеркивалась как хорошая практика, а не мусор каждого блока кода вокруг try / catch. Я твердо верю, что большинство из нас понимают важность этого, однако я вижу, что люди по-прежнему используют анти-паттерн catch-log-rethrow главным образом потому, что для облегчения устранения неполадок во время любого исключения они хотят регистрировать больше информации, специфичной для контекста (пример: параметры метода прошло), и путь заключается в том, чтобы обернуть метод вокруг try / catch / log / rethrow.

public static bool DoOperation(int num1, int num2)
{
    try
    {
        /* do some work with num1 and num2 */
    }
    catch (Exception ex)
    {
        logger.log("error occured while number 1 = {num1} and number 2 = {num2}"); 
        throw;
    }
}

Есть ли правильный способ достичь этого, сохраняя при этом хорошую практику обработки исключений? Я слышал об AOP-фреймворке, таком как PostSharp, но хотел бы знать, есть ли какие-либо недостатки или большие затраты производительности, связанные с этими AOP-фреймворками.

Благодарность!

rahulaga_dev
источник
6
Существует огромная разница между переносом каждого метода в try / catch, регистрацией исключения и разрешением выполнения кода. И попробуйте / поймайте и сбросьте исключение с дополнительной информацией. Во-первых, это ужасная практика. Второй - это прекрасный способ улучшить ваш опыт отладки.
Euphoric
Я говорю, попробуй / поймай каждый метод и просто войди в систему в блоке перехвата и перебрось - это нормально делать?
rahulaga_dev
2
Как указал Амон. Если у вашего языка есть трассировки стека, вход в каждый улов не имеет смысла. Но использование исключения и добавление дополнительной информации - хорошая практика.
Euphoric
1
Смотрите ответ @ Liath. Любой ответ, который я дал, в значительной степени отражал бы его: поймать исключения как можно раньше, и если все, что вы можете сделать на этом этапе, - это записать некоторую полезную информацию, то сделайте это и снова выбросьте. Считать это антипаттерном бессмысленным, на мой взгляд.
Дэвид Арно
1
Liath: добавлен небольшой фрагмент кода. Я использую c #
rahulaga_dev

Ответы:

19

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

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

Пример № 1: справиться с этим

try
{
    doSomething();
}
catch (Exception e)
{
    log.Info("Couldn't do something", e);
    doSomethingElse();
}

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

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

ПРИМЕЧАНИЕ: если вы намеренно игнорируете исключение, я рекомендую предоставить комментарий в пустом предложении catch, в котором четко указано, почему. Это позволяет будущим сопровождающим знать, что это не было ошибкой или ленивым программированием. Пример:

try
{
    context.DrawLine(x1,y1, x2,y2);
}
catch (OutOfMemoryException)
{
    // WinForms throws OutOfMemory if the figure you are attempting to
    // draw takes up less than one pixel (true story)
}

Пример № 2: Добавить дополнительный контекст и бросить

try
{
    doSomething(line);
}
catch (Exception e)
{
    throw new MyApplicationException(filename, line, e);
}

Добавление дополнительного контекста (например, номера строки и имени файла в коде синтаксического анализа) может помочь улучшить возможность отладки входных файлов - при условии, что проблема существует. Это особый случай, поэтому повторная упаковка исключения в «ApplicationException» только для того, чтобы переименовать его, не поможет вам отладить. Убедитесь, что вы добавили дополнительную информацию.

Пример № 3: ничего не делать, за исключением

try
{
    doSomething();
}
finally
{
   // cleanup resources but let the exception percolate
}

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

Берин Лорич
источник
Мне понравилось: « Проблема не в локальном блоке перехвата, проблема в журнале и повторном отбрасывании ». Я думаю, что это имеет смысл для обеспечения более чистого ведения журнала. Но в конце концов это также означает, что все в порядке с try / catch разбросано по всем методам, не так ли? Я думаю, что должно быть какое-то руководство, чтобы обеспечить разумное соблюдение этой практики, а не использование каждого метода.
rahulaga_dev
Я дал руководство в своем ответе. Разве это не отвечает на ваши вопросы?
Берин Лорич
@rahulaga_dev Я не думаю, что есть руководство / серебряная пуля, так что решите эту проблему, так как она сильно зависит от контекста. Не может быть общего руководства, которое говорит вам, где обрабатывать исключение или когда его перебрасывать. IMO, единственное руководство, которое я вижу, - отложить регистрацию / обработку до самого последнего возможного времени и избежать входа в повторно используемый код, чтобы вы не создавали ненужных зависимостей. Пользователи вашего кода не были бы слишком удивлены, если бы вы регистрировали вещи (то есть обрабатывали исключения), не давая им возможности обрабатывать их по-своему. Просто мои два цента :)
andreee
7

Я не верю, что локальные уловы являются анти-паттерном, на самом деле, если я правильно помню, они на самом деле применяются в Java!

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

Лично мне нравится отлавливать ошибки внутри функции, когда я могу выполнить одно из следующих действий:

  • Добавить контекстную информацию (например, состояние объектов или что происходит)
  • Обрабатывать исключение безопасно (например, метод TryX)
  • Ваша система пересекает границу обслуживания и обращается во внешнюю библиотеку или API
  • Вы хотите перехватить и отбросить исключение другого типа (возможно, с оригиналом как внутренним исключением)
  • Исключение было сгенерировано как часть некоторой низкой функциональности фона

Если это не один из этих случаев, я не добавляю локальный try / catch. Если это так, в зависимости от сценария я могу обработать исключение (например, метод TryX, который возвращает false) или перебросить, чтобы исключение было обработано глобальной стратегией.

Например:

public bool TryConnectToDatabase()
{
  try
  {
    this.ConnectToDatabase(_databaseType); // this method will throw if it fails to connect
    return true;
  }
  catch(Exception ex)
  {
     this.Logger.Error(ex, "There was an error connecting to the database, the databaseType was {0}", _databaseType);
    return false;
  }
}

Или пример повторного броска:

public IDbConnection ConnectToDatabase()
{
  try
  {
    // connect to the database and return the connection, will throw if the connection cannot be made
  }
  catch(Exception ex)
  {
     this.Logger.Error(ex, "There was an error connecting to the database, the databaseType was {0}", _databaseType);
    throw;
  }
}

Затем вы ловите ошибку в верхней части стека и представляете пользователю удобное сообщение.

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

Вы не упомянули, на каком языке вы работаете, но были разработчиком .NET и видели это слишком много раз, чтобы не упоминать об этом.

НЕ ПИШИТЕ:

catch(Exception ex)
{
  throw ex;
}

Использование:

catch(Exception ex)
{
  throw;
}

Первый сбрасывает трассировку стека и делает ваш улов верхнего уровня совершенно бесполезным!

TLDR

Локальный отлов не является анти-паттерном, он часто может быть частью дизайна и может помочь добавить дополнительный контекст к ошибке.

Liath
источник
3
Какой смысл регистрировать в перехвате, когда тот же самый регистратор будет использоваться в обработчике исключений верхнего уровня?
Эйфорическая
У вас может быть дополнительная информация (например, локальные переменные), к которой у вас не будет доступа вверху стека. Я обновлю пример, чтобы проиллюстрировать.
Лиаф
2
В этом случае выведите новое исключение с дополнительными данными и внутренним исключением.
Euphoric
2
@Euphoric Да, я видел, что это тоже сделано, лично я не фанат, однако, поскольку он требует от вас создавать новые типы исключений почти для каждого отдельного метода / сценария, который, как я считаю, требует много времени. Добавление здесь строки журнала (и, возможно, еще одной в верхней части) помогает проиллюстрировать поток кода при диагностике проблем
Liath
4
Java не заставляет вас обрабатывать исключение, она заставляет вас знать об этом. Вы можете либо поймать его и сделать что-либо, либо просто объявить это как нечто, что может выдать функция, и ничего не делать с ней в функции .... Немного придираться к довольно хорошему ответу!
Newtopian
4

Это во многом зависит от языка. Например, C ++ не предлагает трассировки стека в сообщениях об ошибках исключений, поэтому может помочь отслеживание исключения с помощью частых перехватов-повторений-повторов. Напротив, Java и подобные языки предлагают очень хорошие трассировки стека, хотя формат этих трасс стека может быть не очень настраиваемым. Поймать и повторно выбросить исключения в этих языках совершенно бессмысленно, если вы действительно не можете добавить какой-то важный контекст (например, связать низкоуровневое исключение SQL с контекстом операции бизнес-логики).

Любая стратегия обработки ошибок, реализованная с помощью отражения, почти всегда менее эффективна, чем встроенная в язык функциональность. Кроме того, повсеместное ведение журналов неизбежно снижает производительность. Таким образом, вам необходимо сбалансировать поток получаемой информации с другими требованиями к этому программному обеспечению. Тем не менее, такие решения, как PostSharp, которые построены на инструментальных средствах уровня компилятора, обычно работают намного лучше, чем отражение во время выполнения.

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

Вход в бизнес-логику гораздо важнее, чем вход в служебные функции. А сбор стековых следов реальных отчетов о сбоях (для которых требуется только регистрация на верхнем уровне процесса) позволяет вам находить области кода, где регистрация будет иметь наибольшее значение.

Амон
источник
4

Когда я вижу try/catch/logв каждом методе, это вызывает опасения, что разработчики понятия не имели, что может или не могло произойти в их приложении, предполагали худшее и упреждающе регистрировали все везде из-за всех ошибок, которые они ожидали.

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

Код, который бросает исключения, может быть более полезным, чем избыточный код, который перехватывает и регистрирует исключения. Если вы генерируете исключение со значимым сообщением, когда метод получает неожиданный аргумент (и регистрируете его на границе службы), это гораздо более полезно, чем немедленная регистрация исключения, вызванного как побочный эффект недопустимого аргумента, и необходимость угадывать, что его вызвало ,

Нули являются примером. Если вы получаете значение в качестве аргумента или результата вызова метода, и оно не должно быть нулевым, тогда генерируйте исключение. Не просто записать получившиеся NullReferenceExceptionброшенные пять строк позже из-за нулевого значения. В любом случае вы получаете исключение, но один говорит вам что-то, а другой заставляет вас искать.

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

Скотт Ханнен
источник
Спасибо Скотт. Выступление, которое вы сделали « Если вы бросаете исключение со значимым сообщением, когда метод получает неожиданный аргумент (и записывает его на границе службы) », действительно поразило меня и помогло мне визуализировать ситуацию, парящую вокруг меня в контексте аргументов метода. Я думаю, что имеет смысл иметь безопасную оговорку и выдавать ArgumentException в этом случае, а не полагаться на
перехват
Скотт, у меня такое же чувство. Всякий раз, когда я вижу log и rethow только для регистрации контекста, я чувствую, что разработчики не могут контролировать инварианты класса или не могут защитить вызов метода. Вместо этого каждый метод, описанный выше, обернут в аналогичную команду try / catch / log / throw. И это просто ужасно.
Макс
2

Если вам нужно записать контекстную информацию, которой еще нет в исключении, вы заключите ее в новое исключение и предоставите исходное исключение как InnerException. Таким образом, у вас все еще сохраняется исходная трассировка стека. Так:

public static bool DoOperation(int num1, int num2)
{
    try
    {
        /* do some work with num1 and num2 */
    }
    catch (Exception ex)
    {
        throw new Exception("error occured while number 1 = {num1} and number 2 = {num2}", ex);
    }
}

Второй параметр Exceptionконструктора обеспечивает внутреннее исключение. Затем вы можете записать все исключения в одном месте, и вы все равно получите полную трассировку стека и контекстную информацию в той же записи журнала.

Возможно, вы захотите использовать пользовательский класс исключений, но смысл тот же.

try / catch / log / rethrow - беспорядок, потому что он приведет к запутанным журналам - например, что если в другом потоке произойдет другое исключение между записью контекстной информации и фактическим исключением в обработчике верхнего уровня? Попытка / ловить / выбросить - это нормально, если новое исключение добавляет информацию к оригиналу.

JacquesB
источник
Как насчет оригинального типа исключения? Если мы завернем, это уйдет. Это проблема? Например, кто-то полагался на исключение SqlTimeoutException.
Макс
@Max: исходный тип исключения все еще доступен как внутреннее исключение.
JacquesB
Это то, что я имею в виду! Теперь все в стеке вызовов, которые ловили за SqlException, никогда не получат его.
Макс
1

Само исключение должно предоставлять всю информацию, необходимую для правильной регистрации, включая сообщение, код ошибки и что нет. Поэтому не должно быть необходимости перехватывать исключение только для того, чтобы сбросить его или выдать другое исключение.

Часто вы видите шаблон нескольких исключений, перехваченных и переброшенных как общее исключение, таких как перехват исключений DatabaseConnectionException, InvalidQueryException и InvalidSQLParameterException и повторное выбрасывание DatabaseException. Хотя, к этому, я бы сказал, что все эти конкретные исключения в первую очередь должны быть производными от DatabaseException, поэтому повторное отбрасывание не требуется.

Вы обнаружите, что удаление ненужных предложений try catch (даже тех, которые предназначены исключительно для ведения журнала) на самом деле сделает работу проще, а не сложнее. Только места в вашей программе, которые обрабатывают исключение, должны регистрировать исключение, и, если все остальное терпит неудачу, обработчик исключений в рамках всей программы для одной последней попытки зарегистрировать исключение перед грациозным выходом из программы. Исключение должно иметь полную трассировку стека, указывающую точную точку, в которой было сгенерировано исключение, поэтому часто нет необходимости обеспечивать «контекстную» регистрацию.

Тем не менее, АОП может быть быстрым решением для вас, хотя обычно это влечет за собой небольшое замедление. Я бы посоветовал вам вместо этого полностью удалить ненужные предложения try catch, в которых ничего не добавлено.

Нил
источник
1
« Само исключение должно предоставлять всю информацию, необходимую для правильного ведения журнала, включая сообщение, код ошибки и что нет ». Они должны это делать, но на практике они не ссылаются на исключения Null, являющиеся классическим случаем. Я не знаю ни одного языка, который скажет вам переменную, которая вызвала его внутри сложного выражения, например.
Дэвид Арно
1
@DavidArno Правда, но любой контекст, который вы могли бы предоставить, тоже не может быть таким конкретным. В противном случае вы бы имели try { tester.test(); } catch (NullPointerException e) { logger.error("Variable tester was null!"); }. Трассировка стека в большинстве случаев достаточна, но при отсутствии такого типа тип ошибки обычно адекватен.
Нил