2 признака кода с душком: убей его и лови всё молча

Знакомы ли вы с понятием “Код с душком”? Если нет, то коротко говоря — это плохой код. Термин был введен Мартином Фаулером в его книге Рефакторинг. Улучшение существующего кода и с тех пор очень активно используется в программерских кругах. Да и менеджерам, управляющим разработкой ПО, было бы неплохо знать признаки кода с душком, чтобы уметь распознавать качество кода, создаваемого командой.

У Фаулера в книге целая большая глава посвящена признакам кода с душком.

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

  1. Код с TerminateThread или аналогами. Условное название “Убей его”
  2. Код с пустым блоком catch(…) или аналогами типа catch (Throwable t) в Java. Условное название “Лови всё молча”

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

“Плохость” кода обычно значит, что программист не понимает, что происходит или будет происходить в коде, поэтому он оборачивает его в catch(…). Либо же он сомневается, что поток достаточно хорошо написан и он может зависнуть, так что программист использует TerminateThread.

Причина в обоих случаях — нежелание разбираться и делать все правильно.
Вначале кажется, что и TerminateThread и catch(…) справляются со своей задачей и все отлично. Но на самом деле они просто скрывают реальные проблемы и эти проблемы в итоге остаются неисправленными и, например, программа начинает падать или зависать в самых неожиданных местах.

Рассмотрим эти два признака по отдельности.

1. TerminateThread с аналогами

Вариантов того, как может навредить TerminateThread — множество.

MSDN нам внушает:

TerminateThread is a dangerous function that should only be used in the most extreme cases. You should call TerminateThread only if you know exactly what the target thread is doing, and you control all of the code that the target thread could possibly be running at the time of the termination. For example, TerminateThread can result in the following problems:
* If the target thread owns a critical section, the critical section will not be released.
* If the target thread is allocating memory from the heap, the heap lock will not be released.
* If the target thread is executing certain kernel32 calls when it is terminated, the kernel32 state for the thread’s process could be inconsistent.
* If the target thread is manipulating the global state of a shared DLL, the state of the DLL could be destroyed, affecting other users of the DLL.

Но даже эти 4 страшных последствия не останавливают многих программистов от применения TerminateThread (а может просто никто их не читает).

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

Зависший и потом уничтоженный c помощью TerminateThread поток уже убит и нет никаких шансов понять, что же там пошло не так! При этом то, что остальные потоки зависли — это отлично, так как указало на проблему. Обычно же этот поток убивался безболезненно и никто даже не знал о проблеме. Но как всегда в одном из миллиона запусков дедлок случался.

В итоге, если убрать TerminateThread и дождаться зависания потока, то можно исправить реальную проблему в этом самом потоке и программа перестает зависать при каждом миллионном запуске.

Если вы думаете, что зависание на каждом миллионном запуске — это нечасто, то представьте, что ваш продукт установлен на 20 млн. компьютеров и программа запускается 10 раз в день — в итоге у вас будет 200 зависаний в день и 70000 в год, многие из которых трансформируются в звонки в техподдержку или в баг-репорты, которые кто-то будет изучать и пытаться исправить. Много работы из-за временной слабости программиста.

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

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

В общем, как всегда — выбор за вами.

2. Код с catch(…) или аналогами типа catch (Throwable t) в Java

Про использование catch(…) я встречал несколько холиваров, где кто-то доказывал его полезность, а другие — вредность.

В целом могу согласиться, что иногда это может быть полезным. Например, у вас завтра непереносимый релиз, а какой-то кусок кода кидает непонятные исключения и вы не можете их понять и исправить — можно понять catch(…) в этом месте на пару дней. Но его нужно сразу же убрать после релиза. Почему?

Потому что catch(…) прячет ошибки. Даже если вы напишете вывод чего-то в лог в этом блоке catch, то это ничего не даст — контекст ошибки уже потерян. Да, программа не упадет, но станет работать дальше неправильно.

Если вы в каждом блоке catch(…) сохраняете полное состояние ошибки (дамп, калстэк, регистры и т.п.) — это поможет с отладкой, но опять же, сколько места на диске это потребует и какие другие баги и тормоза это добавит в ваш продукт?

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

Я помню один случай, когда такой блок catch(…) с выводом в лог привел к тому, что у одного из игроков в нашу игру лог заполнил весь жесткий диск — 120Гб. При этом понять из этого лога причину ошибки было невозможно, была видна только функция, где была ошибка.

Еще помню, мы как-то использовали catch(…) повсеместно в коде одной игры во время разработки и получали изредка репорты о падениях в самых разных функциях, в которых глюков не было и не могло быть. При этом реальные ошибки маскировались этими catch(…) и в принципе казалось, что все работает стабильно. Но в итоге было принято решение все catch(…) убрать, после чего была пара ужасных недель, когда все начало падать постоянно, зато через эти две недели все реальные ошибки были найдены и исправлены. И код стал гораздо стабильнее. И, главное, мы после этого знали, что отсутствие падений — это не заслуга кучи catch(…), а именно отсутствие критических глюков. Это знание многого стоит!

В качестве заключения хочу предложить вам прямо сейчас открыть свои проекты и поискать там catch(…), TerminateThread, TerminateProcess и т.п. Найдёте — задумайтесь и исправьте.

А также подумайте о внесении запрета на их использование в свои стандарты кодирования. Они же у вас есть, правда?

P.S. Признаков кода с душком можно написать еще очень много. Я выбрал эти два, так как они достаточно спорные и в то же время очевидные. С удовольствием подискутирую на эту тему.

Как вы относитесь к применению TerminateThread и catch(…) в коде?

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