Сия заметка не столь для программистов, многие из которых уже сталкивались с подобным, и только улыбнутся "ну, открыл Америку" - а больше для разного рода менеджеров, заказчиков и всех кто считает что достаточно лишь нанять умных разработчиков, и дело в шляпе. Вот шляпой дело нередко и оборачивается.
Готовится релиз. Сроки подходят. Мне скидывают странный баг: Наше приложение вдруг стало жаловаться на невозможность соединиться с соседним.
А почему не может? Защищённое соединение не устанавливается.
А почему не устанавливается? Файлы сертификатов для этого соединения не удаётся загрузить.
А почему файлы не грузятся? А потому что путь к файлам "отсутствует в конфигурации".
А если руками залезть и глазами посмотреть - присутствует. Чудеса! Эффект Шрёдингера!
О проекте
Приложение - часть большого проекта. В то же время и само приложение не одна большая "программулина", а состоит из нескольких отдельных "сервисов". В сумме это больше 4500 исходных файлов кода, из которых, правда, 2500 это файлы тестов. Из файлов с тестами, правда, примерно половина - пустые, потому что специальная умная утилита создаёт их по шаблону в любой новой директории с исходниками.
Сервисы приложения используют некую "базу данных", её я беру в кавычки т.к. это довольно специфическое и притом небольшое хранилище - но для нашего рассказа это неважно. Знатоки сразу подметят "а хорошо ли что все сервисы вместе ходят в базу?" Действительно, этого часто стараются избегать - но тут сложилось что, в частности, некоторые параметры конфигурации хранятся в этой самой "базе" - вот как раз чтобы все сервисы могли удобно сходить и их прочитать.
И, как видим, один из параметров (один ли?) вдруг не читается.
Расследование показало
Смотрю в код сервиса, который не смог прочесть злосчастный "путь к сертификатам", а там что-то в духе такого (язык и названия изменены):
В одном из файлов инициализирующих сервис вызывается функция для определения пути к сертификатам:
function initSecureConnection() { // ... certificatesPath = certificatesParametersReader.ReadCertificatesPath() if (certificatesPath == ERROR_NOT_FOUND) { // кидаем ошибку } // ... }
Не хухры-мухры, у нас целый вспомогательный класс для чтения параметров связанных с сертификатами - и пути к ним в частности. Как-то так:
class CertificatesParametersReader { // ... function ReadCertificatesPath() { // тут строк 30 // лезем в базу... // устанавливаем сессию... // проверяем разные ошибки... } // ... }
В целом написано всё солидно. Этот класс для чтения параметров густо покрыт "юнит-тестами", в частности только на метод ReadCertificatesPath который нас интересует - тестов штук 7 написано - проверяют разные случаи проблем при обращении к базе и так далее.
В общем, прихожу к выводу что тут проблемы нет. Если нужный параметр в базу записан - то сервис его прочтёт.
Значит его нет, когда его читают.
Но я же его вижу...
Хм... Посмотрим, откуда он в базе появляется. К сожалению эта часть про управление защищёнными соединениями и их сертификатами - это целый отдельный сервис - в общем, функционал мне незнакомый до сих пор - такое случается если проект довольно большой и ты много работаешь в основном с какими-то другими его аспектами.
Найти в проекте из тысяч файлов кто записывает строчку в базу - это всё же не иголку в стоге сена искать. По имени под которым данный параметр в базе находится, отыскиваю нужный файл. Тут всё не так развесисто, просто есть метод для записи:
function fillConfig() { // ... if (fillCertificatesPath(session) == ERROR) { return new Error("error while filling certificates path") } // ... } // ... function fillCertificatesPath(session) { session.WriteData(certificatesPathParamName, certificatesPathParamValue) }
Ну хорошо, включим уровень логгирования на котором видно, когда происходит запись в базу. Перезапустим сервис... Вот, опять ошибка... Смотрим, анализируем... Ну конечно:
чтение параметра
certificatesPathпроизошло в 17:38:24 (и обломалось)запись параметра
certificatesPathпроизошла в 17:38:26 (на 2 секунды позже)
Очевидно, наша архитектура кривовата - сервисы вынуждены стартовать в строго определенной последовательности и не умеют дождаться пока один другому параметры в базе проставит. В общем много способов как это можно было сделать проще и лучше, но мы на них останавливаться не будем.
Потому что проблема даже не в этом!!!
Стал я размышлять как тут лучше поступить - то есть, перебирать в голове упомянутые "много способов". Поскольку речь идёт о релизе, тут надо действовать минимальными и наиболее безобидными изменениями, чтобы не задеть чего-нибудь ещё, чтобы не пришлось много перетестировать.
Самым очевидным кажется подсунуть куда-то вокруг (или внутри) функции чтения повтор с задержкой, ну в духе:
function ReadCertificatesPath() { // вынесем тело в отдельную функцию ниже, а здесь будет цикл delay = 100 * millisecond startTime = currentTime() while (currentTime() - startTime < 5000 * millisecond) { path = readCertificatesPathAttempt() if (path != ERROR) { return path } sleep(delay) delay = delay * 3 / 2 } } function readCertificatesPathAttempt() { // прежние 30 строк тут }
То есть пробуем вычитывать параметр в течение 5 секунд, например, и если это хоть раз удастся - возвращаем то что прочлось. Иначе делаем паузу (с постепенно увеличивающейся задержкой, чтобы поменьше "бомбить" базу) и повторяем попытку.
Но прежде чем делать это...
Дай-ка, пожалуй, посмотрю, а откуда изначально вообще этот путь к сертификатам приходит, откуда он передаётся в функцию сохранения. Помните вот это:
session.WriteData(certificatesPathParamName, certificatesPathParamValue)
Вот что это за certificatesPathParamValue - при первом чтении я не обратил внимания - а сейчас глянул на определение (оно, как модно, оказывается в другом файле):
const certificatesPathParamValue = "/etc/trusted/common.crts"
То есть это константа. Вся эта свистопляска с методами для записи значения в базу, с мудрёным классом для его вычитывания, с тестами на этот класс - ради передачи "захардкоженного" значения, которому не судьба в жизни никогда поменяться.
Я ещё подумал - может, чего-то не догоняю? Может этот параметр какими-то силами можно проставить извне, поменять путь к сертификатам (зачем? даже звучит странно - но мало ли...)
Ан нет, ведь вычитывается он только на старте, а при старте наших сервисов этот "конфигурационный" раздел базы очищается и перезаписывается - ну, сами видите, на старте вызывается fillCertificatesPath - и затрёт любые изменения.
Анализ
В "релизной" ветке допустимы только минимальные изменения - я просто меняю код так чтобы в случае ошибки чтения параметра сервис не падал с ошибкой, а просто возвращал такую же константу (это очень глупо звучит, но код релиза требует бережного обращения).
В основной ветке, конечно, я выпиливаю код относящийся к чтению и записи этого параметра - в сумме уходит несколько сотен строк, вместе с "тестами" (которые проверяют класс, который вычитывает из базы записанную туда константу) - поскольку этот же код ещё и раздублирован с небольшими изменениями в другие сервисы которым тоже нужен этот путь к сертификатам (они тоже могут упасть, поэтому в релизной ветке делаю поправку и для них).
Как же это получилось? Откуда такой сложный код для выдачи константы? Можно подумать что это результат исторический - быть может когда-то давно это не было константой... И только после в результате рефакторинга сложилось так, как сложилось.
Но нет, код проекта ещё пока в состоянии "стартапа", и хотя он пишется уже не первый год, у него нет исторической коньюнктуры. Более того - я уже видел (и выпиливал) подобные штуки в нём пару месяцев назад - просто тогда они не доводили до багов. Просто увидел, удивился, удалил.
Итак, как же это получилось?
Это не от глупости ведь. Наоборот видно, код развесистый, сложный, мудрёный - настолько что вот и тестами его солидно покрыть пришлось. Глупец так не сделает, так только умный сделает. Может быть, слишком умный. В разработке было некое поветрие, увлечение "паттернами проектирования" - то есть, "шаблонами", якобы, полезными - и вот здесь угадываются черты этого "шаблонного мышления" (в обоих смыслах).
Нельзя просто так взять и вычитать параметр.
Нужен класс Reader к параметру.
Нужен интерфейс на этот класс.
Нужны тесты на этот класс.
Нужны "моки" на этот интерфейс.
И только одна маленькая деталь упущена - параметр вообще не требовалось вычитывать. Но поскольку проект такой здоровый, поскольку запись в другом сервисе, поскольку над кодом работают человек 20 программистов - это прошло незамеченным.
Незамеченным? Да нет, ведь, как я сказал, это не случайность - подобных константных параметров в коде не так уж мало...
Это образ мысли.
Со стороны (менеджерам, заказчикам) кажется что проект такой большой потому что в нём много функционала. Я же на глазок оцениваю что тут фактической работы на 3 человек на полгода - всё остальное - это вот подобная "развесистая клюква".
И это не в какой-то "сомнительной шараге" - контора солидная, в топах разных рейтингов (и на хабре / хабр-карьере в том числе). Тысячи человек народу. Они пишут миллионы файлов исходного кода. Представьте процент этой "клюквы" в этих миллионах файлов. Пересчитайте это во время и деньги.
Хуже того - как видим, временами эта ересь ведёт к сбоям - хорошо что поймали до релиза, а не после. Было бы сложнее, больнее, дольше и дороже разбираться.
И ведь это не диверсанты делают, это не со зла. Из лучших побуждений - а вдруг когда-то понадобится нарастить этот функционал - а у нас уже все готово.
А вдруг - не понадобится? Почему бы не добавлять код тогда, когда он нужен. Это же код, его можно редактировать!
Не зря говорится: Don't be too clever.