— Мы забрели в зону с сильным магическим индексом-объяснил он, — Когда-то давно здесь образовалось мощное магическое поле.
– Вот именно, — ответил проходящий мимо куст.
Терри Пратчетт, Цвет волшебства
Поддерживать некрасивый код неприятно. В некрасивом коде сложнее разобраться, он чаще бывает устаревшим и зачастую содержит ошибки. Однако это честная неприятность — ты сразу знаешь, что с кодом не всё впорядке и пишешь дополнительные тесты перед изменением, несколько раз проверяешь, закладываешь в оценках время на то, чтобы всё починить.
Красивый код в этом отношении другой: ты его легко читаешь, в нём обычно используются новые технологии и ты охотно веришь, что вот он-то работает оптимально и в нём нет ошибок. Хотя это-то как раз легко может быть неправдой.
В этой статье я покажу что верить нельзя никакому коду (все лгут) и продемонстрирую несколько интересных ошибок.
Guava: как написать меньше кода и потратить больше ресурсов
Guava — это библиотека базовых методов и объектов, созданная Гуглом в качестве альтернативы стандартным библиотекам и обладающая множеством полезных функций. В частности в Guava реализованы библиотеки для работы с коллекциями, которые позволяют упростить манипуляцию с данными. Эта история покажет что эту простоту очень легко использовать во зло.
Пусть нам надо реализовать сервис, который занимается в основном преобразованиями данных: запрашивает данные у внешней системы, оповещает другую внешнюю систему и отдаёт данные тому, кто их запрашивал. Реализация может выглядеть примерно так:
private final static Predicate<Entity> checkPredicate = entity -> entity.performCheck();
private final static Function<Entity, Integer> mapToIdFunction = entity -> entity.getId();
private final Function<Integer, Entity> lookupFunction = id -> storageService.lookupEntity(id);
@Override
public ImmutableList<Entity> getCheckedEntityByIds(List<Integer> ids) {
logger.info("Received " + ids.size() + " ids.");
List<Entity> uncheckedEntities = Lists.transform(ids, lookupFunction);
Collection<Entity> filteredEntities = Collections2.filter(uncheckedEntities, checkPredicate);
notificationService.sendUpdate(Collections2.transform(filteredEntities, mapToIdFunction));
logger.info("Got " + filteredEntities.size() + " entities.");
return ImmutableList.copyOf(filteredEntities);
}
Согласитесь, написано достаточно лаконично и можно легко понять, что происходит, просто читая исходный код. Есть только одна проблема — этот красивый и лаконичный код содержит ошибку, которая приводит к бессмысленной трате ресурсов и может привести к внутренним ошибкам в данных. Ошибку легко продемонстрировать таким тестом:
@Before
public void setup() {
storageService = new StorageService() {
@Override
public Entity lookupEntity(int id) {
lookupCounter++;
return new Entity(id);
}
};
notificationService = new NotificationService() {
@Override
public void sendUpdate(Collection<Integer> ids) {
System.out.println(ids);
}
};
guavaServiceImpl = new GuavaServiceImpl(storageService, notificationService);
}
@Test
public void testGuava() {
List<Integer> ids = new ArrayList<>();
for (int i = 0; i < 100; ++i) {
ids.add(i);
}
guavaServiceImpl.getCheckedEntityByIds(ids);
Assert.assertEquals(100, lookupCounter);
}
java.lang.AssertionError: expected:<100> but was:<300>
at org.junit.Assert.fail(Assert.java:88)
Как несложно видеть, результат теста неутешительный — мы делаем в три раза больше запросов, чем разумно. Это вызвано тем, что соответсвующие методы на самом деле не трансформируют\фильтруют\etc, а создают соответсвующую обёртку. Т.е. для получения информации, к примеру, о длине коллекции нам прийдётся перепроверять все элементы из более верхней коллекции (трансформирующей), которая в свою очередь будет трансформировать элементы каждый раз при запросе.
Так что в лучшем случае мы получим потерю производительности из-за скрытой стоимости простых операций ~O(n * m), где n — число элементов, а m — вложенность коллекций, а в худшем это приведёт у утечкам памяти и неконтролируемым запросам к другим сервисами т.к. даже отфильтрованная коллекция с несколькими элементами будет хранить ссылки на все изначальные элементы.
Совет по использованию Guava: даже если результаты трансформации и фильтрации выглядят как коллекции и крякают как коллекции, считать коллекциями их не стоит, они являются одноразовыми и перед практически любым использованием стоит скопировать их содержимое в другую коллекцию. К примеру хорошая практика использовать ImmutableList.copyOf() — это сможет защитить и от других проблем.
Google App Engine: почему полезно не верить документации
Все лгут.
Доктор Хаус
Google App Engine — достаточно удобная платформа для разработки и развёртывания облачных приложений, с возможностями для автоматического выделения и распределения ресурсов при высокой нагрузке и быстрой работы с различными сервисами, как то база данных и файловое хранилище. Эта история покажет, какие подводные камни есть у простоты интеграции между различными компонентами.
Главный герой этой истории — средний по размерам проект на базе App Engine с одной базой данных Cloud SQL (Maria DB), которую он мапил в бизнес сущности с помощью Hibernate.
Проект развивался, база росла и в один прекрасный момент наш будущий пациент просто упал. Падение сопровождались пустотой в логах и огромным числом коннектов к базе данных (при всего нескольких экземплярах приложения). Также в логах базы данных обнаружилось несколько неудачных бейкапов. После рестарта базы данных приложение стало работать как ни в чём не бывало.
Самый напрашивающийся диагноз — это
Лирическое отступление об mysql binlog
MySQL поддерживает несколько видов логов. В бинарные логи попадают только транзакции, которые изменяют базу данных, благодаря чему он работает достаточно быстро даже для использования на продакшн сервере.
Для включения его необходимо запустить инстанс с атрибутом --log-bin[=base_name].
Проверить что бинарные логи пишутся можно таким запросом:
Прочитать полученые результаты можно с помощью утилиты mysqlbinlog
Для включения его необходимо запустить инстанс с атрибутом --log-bin[=base_name].
Проверить что бинарные логи пишутся можно таким запросом:
SHOW VARIABLES LIKE 'log_bin';
SHOW BINARY LOGS;Прочитать полученые результаты можно с помощью утилиты mysqlbinlog
Однако ни лечение, ни анализы не помогли — падение повторилось, а бинарные логи не показали никаких подозрительных транзакций.
После тщательной перепроверки всего, связанного с базой данных, причина падений таки была найдена и она оказалась вызвана следованием рекомендациям из документации App Engine. В частности, в случае приложения и базы данных на App Engine, документация советует открывать новый коннект к базе данных каждый раз при получении нового запроса, т.к. открытие нового коннекта дешевле чем поддержание старого:
How to best manage your database connections depends on your use case. For example, if the time to create a new database connection is greater than checking and reusing an existing connection, then we recommend you use connection pooling. Conversely, if the time to create a new connection is about the same as testing if an existing connection is alive and reusing it, then we recommend that you create a new connection to service each HTTP request, and reuse it for the duration of the request. In particular, the latter case may apply when you connect from Google App Engine to Google Cloud SQL.
Google Cloud SQL FAQ
Однако в связке с Hibernate такой подход привёл к достаточно печальным результатам: в этом приложении есть несколько достаточно тяжёлых запросов на выборку данных из таблицы, которые обычно прорабатывают нормально. Но в случае нескольких параллельных запросов они могут выполнятся дольше минуты, что приводит к автоматическому обрыву запроса средствами App Engine. Код, который ждёт результатов запроса на Java стороне перестаёт выпоняться, но сессия на стороне базы данных продолжает обработку результата тормозя следующие запросы, в результате чего и получается своеобразный «снежный ком».
Для полноты истории стоит рассказать, что всё закончилось хорошо благодаря настройке пула коннектов. В случае Hibernate это можно сделать при помощи замены свойства
<connection.pool_size>0</connection.pool_size>Предупреждение: встроенный в Hibernate пул коннектов использовать не стоит!
В документации Hibernate прямо говорится, что его можно использовать только для тестовых целей:
Hibernate's own connection pooling algorithm is, however, quite rudimentary. It is intended to help you get started and is not intended for use in a production system, or even for performance testing. You should use a third party pool for best performance and stability. Just replace the hibernate.connection.pool_size property with connection pool specific settings. This will turn off Hibernate's internal pool. For example, you might like to use c3p0.
Hibernate API documentation
Небольшое заключение
Эти две истории символизируют, что никакая простота и красота кода не даётся бесплатно. Помните — все лгут и для того, чтобы ваше приложение работало так хорошо как вы хотите, вы должны точно понимать что на самом деле делает ваш красивый код и та магия, которую он использует.
Комментарии (23)
M_Muzafarov
14.10.2015 08:36+12Бейкап? это так называются битые бэкапы?
MaximChistov
14.10.2015 09:18+1Это же восьмая Java? тогда почему не так?
@Override public ImmutableList<Entity> getCheckedEntityByIds(List<Integer> ids) { logger.info("Received " + ids.size() + " ids."); Collection<Entity> filteredEntities = ids.stream() .map(id -> storageService.lookupEntity(id)) .filter(Entity::performCheck) .collect(Collectors.toCollection()); notificationService.sendUpdate(filteredEntities.stream() .map(Entity::getId) .collect(Collectors.toCollection())); logger.info("Got " + filteredEntities.size() + " entities."); return ImmutableList.copyOf(filteredEntities); }
DemetrNieA
14.10.2015 10:15-4Потому что тогда бы этой истории не было бы — stream api действительно имеет более предсказуемое поведение, хотя и работает в среднем медленнее guava (в случае завершающего копирования).
Сам пример, очевидно, от версии Java слабо зависит. Писался на 8й для краткости исходного кода.MaximChistov
14.10.2015 12:10+3stream api действительно имеет более предсказуемое поведение, хотя и работает в среднем медленнее guava (в случае завершающего копирования)
А можно пруфы? =/
EngineerSpock
14.10.2015 10:04+3Если исключить то, что вы до конца не разобрались в исходных доках, то я бы обратил внимание на следующую мысль. Красивый код — не самоцель, он ещё и работать должен. Врёт не красивый код, а код, который, эм… собственно врёт.
mayorovp
14.10.2015 10:27Что-то я не понимаю. При автоматическом обрыве выполнения кода должны быть закрыты все используемые ресурсы, в том числе соединение с БД. Закрытие соединения с БД должно остановить все выполняемые запросы.
Если в этом механизме что-то поломалось, то при чем тут пулы соединений?DemetrNieA
14.10.2015 10:51При автоматическом обрыве запроса обрывается выполнение этого запроса. В случае, если запрос был «убит» это может вызвать повисший запрос, который конечно откатится, но уже спустя некоторое время.
Пулы соединений как раз гарантируют, к примеру, что один инстанс не будет открывать слишком много соединений.mayorovp
14.10.2015 15:44Что такое «обрыв выполнения»? Если это Thread.stop — то он возбуждает ошибку ThreadDeath, которая приводит, в том числе, к выполнению finally-блоков.
Если всюду где надо использовать примитивы вроде try-with-resources, и при этом не выполнять работы в конструкторах — то все запросы должны нормально отмениться.
gurinderu
14.10.2015 12:37+2C Guava я бы написал так. Итого 1 итерация.
@Override public ImmutableList<Entity> getCheckedEntityByIds(List<Integer> ids) { logger.info("Received " + ids.size() + " ids."); ImmutableList<Entity> result = FluentIterable .from(ids) .transform(lookupFunction) .filter(checkPredicate) .transform(mapToIdFunction) .toList(); logger.info("Got " + result.size() + " entities."); return result; }MaximChistov
14.10.2015 12:57неа
там в нотификейшн айдишники идут, а в результат — полноценные объектыgurinderu
14.10.2015 14:02Логично. Убрать transform(mapToIdFunction) и вынести логику notificationService на уровень выше нужно. Ибо с названием метода данный call никак не связан.
MaximChistov
14.10.2015 14:55на уровень выше не получится — в него отправляются не все айдишники со входа, а только отфильтрованные
Regis
В Guava на каждом методе c transform в JavaDoc написано, что transform создает View, а не новую коллекцию. В документации и примеры на это, и пояснения для чего так, и что делать если там нужна именно копия. Не читаем, осуждаем. ССЗБ? Да.
Второй пример — вообще смешно. Вы выделили жирным «then»часть предложения, но почему-то не «if» :)
Ваш случай подпадал под один вариант, а вы выбрали другой. И пишете, что документация плохая :)
PS; Вместо того, чтобы использовать коннекшн пул нулевого размера — может лучше его всё же полностью убрать? :)
PS2: Статью следовало бы назвать «Даже усли использовать хорошие библиотеки, вы всё равно можете выстрелить себе в ногу» :)
DemetrNieA
Да, написано, но абсолютно неочевидно из сигнатуры метода, в результате чего такая ошибка мне встречалась крайне часто (причём и у опытных людей). Мораль в результате именно такая: не верь тому, как оно выглядит, обязательно понимай как работает.
Читаем, обсуждаем, передаём опыт будущим поколениям.
Потому что после then написан ещё пример, когда это условие верно — при связи между AppEngine и CloudSQL.
Почему Вы так считаете? Скорость открытия соединения в описываемом случае действительно мала, так что проект подпадал как раз под этот вариант.
Не могли бы Вы объяснить, что Вы имеете ввиду? Пулл нулевого размера — это и есть дефакто отсутствие пулинга. (если просто убрать настройку пулинга, то будет использоваться default value 1 и это как раз то, что делать совсем не стоит).
И более того, они могут помочь вам в этом, так как вы не будете осознавать, что стреляете.
Avers
Получается «я книжку не читал, но она мне не понравилась». Если не читать документацию, а опираться на сигнатуры методов, то это уже не программирование/освоение библиотеки, а метод тыка в поисках «вроде бы работающего кода».
Regis
Просмотрел бегло актуальные доки — судя по всему, со времен отделения c3p0 от самого Hibernate, hibernate.connection.pool_size со значением 0 именно отключает встроенный пул и вроде бы действительно отключает все дополнительные механизмы, нужные для пуллинга.
bohdan4ik
> Потому что после then написан ещё пример, когда это условие верно — при связи между AppEngine и CloudSQL.
Там написано, что условие может быть (а может и не быть) истинным в данной связке. Снова же, в зависимости от конкретного случая.
> The key words «MUST», «MUST NOT», «REQUIRED», «SHALL», «SHALL
> NOT», «SHOULD», «SHOULD NOT», «RECOMMENDED», «MAY», and
> «OPTIONAL» in this document are to be interpreted as described in
> RFC 2119.
> www.ietf.org/rfc/rfc2119.txt
DemetrNieA
Это инструкции для IETF. При написании документации к cloud sql им следовать, вообще говоря, не обязаны.
В данном случае имеет место рекомендация (и условие действительно выполняется).
bohdan4ik
Вообще говоря, не обязаны, но в данном случае описание «may» более, чем подходит для выбранной цитаты. Даже дословный перевод это «может», а не «должен».
Рекомендации на то и рекомендации, что не обязательны к исполнению.
Я покидаю дискуссию, поскольку далее мы только будем толочь воду в ступе.
Спасибо.
P.S.: не поймите неправильно, я бы тоже ругался, что документация порекомендовала и рекомендация привела к неожиданным результатам, но на меня мои доводы подейстовали бы (как-то была похожая ситуция) :)