Код-ревью: фрагменты 7-12

Вторая половина: фрагменты 7-12, от сравнения ошибок через == до defer внутри цикла. Каждый факт уже проверен реальным go run/go vet/-race при первом написании этого материала — цифры и вывод не выдуманы.

Как тренироваться: на каждый фрагмент — найти ВСЕ баги (их может быть несколько) и проговорить вслух механизм поломки, как будто объясняешь интервьюеру, прежде чем читать разбор ниже. Не останавливайся на первом найденном баге — это самая частая ошибка при разборе чужого кода под давлением времени.

Фрагмент 7

var ErrNotFound = errors.New("not found")

func FindUser(db *sql.DB, id int) (*User, error) {
    // ... запрос к базе ...
    if noRows {
        return nil, fmt.Errorf("FindUser(%d): %w", id, ErrNotFound)
    }
    // ...
}

func Handler(db *sql.DB, id int) {    _, err := FindUser(db, id)    if err == ErrNotFound {          // сравнение через ==        respondNotFound()        return    }    if err != nil {        respondServerError()    }}

func Handler(db *sql.DB, id int) {    _, err := FindUser(db, id)    if errors.Is(err, ErrNotFound) { // ИСПРАВЛЕНО: разворачивает цепочку %w        respondNotFound()        return    }    if err != nil {        respondServerError()    }}

Баг — сравнение обёрнутой ошибки через ==, реально проверено: fmt.Errorf("...: %w", ErrNotFound) создаёт новое значение ошибки, которое лишь «оборачивает» ErrNotFound внутри себя — это не тот же указатель/значение. Реальный прогон: err == ErrNotFound даёт false, хотя ошибка семантически именно ErrNotFound. Итог — код никогда не попадает в ветку respondNotFound() и вместо корректного 404 всегда отвечает 500.

Механизм. error в Go — интерфейс, а == для интерфейсных значений сравнивает пару (динамический тип, значение) на точное совпадение. fmt.Errorf("...: %w", ErrNotFound) возвращает не ErrNotFound, а НОВОЕ значение другого конкретного типа (*fmt.wrapError), которое просто хранит ссылку на ErrNotFound внутри себя и умеет её отдать через метод Unwrap() error. Для == это два совершенно разных значения — сам факт, что одно логически "содержит" другое, для оператора сравнения не имеет значения, он не умеет заглядывать внутрь.

errors.Is, в отличие от ==, устроен как цикл: сравнивает цель с самим err, и если не совпало — вызывает Unwrap() и пробует снова, пока Unwrap() не вернёт nil (конец цепочки) или не найдётся совпадение.

Что случится в проде. Хендлер, который должен вернуть 404 "пользователь не найден", вместо этого ВСЕГДА возвращает 500 "внутренняя ошибка сервера" — для любого несуществующего ID. С точки зрения клиента API это выглядит как систематический сбой сервиса на самой обычной операции (поиск несуществующей записи — это ожидаемый случай, а не авария), при этом в логах сервера накапливается лавина 500-х ошибок по некорректным, но абсолютно штатным запросам клиентов, что маскирует реальные проблемы среди шума ложных алармов.

Как заметить на ревью. Сигнал — сравнение переменной типа error с sentinel-ошибкой через == или !=, если где-то выше по стеку вызовов эта ошибка оборачивается через fmt.Errorf с %w (что почти всегда так, если в коде вообще принято добавлять контекст к ошибкам). Практическое правило: ==/!= для сравнения ошибок можно использовать только если ТОЧНО известно, что ошибка никогда не оборачивается по пути — в остальных случаях по умолчанию errors.Is/errors.As.

Фрагмент 8

func Handler(w http.ResponseWriter, r *http.Request) {    if !isValid(r) {        w.WriteHeader(http.StatusBadRequest)    }    w.WriteHeader(http.StatusOK)    json.NewEncoder(w).Encode(result)}

func Handler(w http.ResponseWriter, r *http.Request) {    if !isValid(r) {        w.WriteHeader(http.StatusBadRequest)        return // ДОБАВЛЕНО: не проваливаться дальше после ответа об ошибке    }    w.WriteHeader(http.StatusOK)    json.NewEncoder(w).Encode(result)}

Баг — второй WriteHeader вызывается безусловно, реально проверено на настоящем сервере: если запрос невалиден, код сначала пишет 400, а затем сразу же, без return, ещё и 200. Go на реальном сервере логирует предупреждение http: superfluous response.WriteHeader call, а клиент видит первый код (400) — но тело ответа при этом всё равно успешно пишется дальше (json.NewEncoder(w).Encode(result) не проверяет статус) — клиент получает статус 400 с телом, которое выглядит как успешный ответ. Расхождение статуса и тела — реально путающий баг для того, кто дебажит клиента.

Механизм. HTTP-статус — это часть заголовков ответа, а заголовки в протоколе HTTP отправляются клиенту ОДИН РАЗ, до тела, и не могут быть изменены после того, как ушли по сети. http.ResponseWriter в Go отражает это ограничение: первый вызов WriteHeader (или первая запись через Write, которая неявно вызывает WriteHeader(200), если он ещё не был вызван) фиксирует статус безвозвратно — реальный статус-код, который получит клиент, это именно первый вызов, 400.

Второй WriteHeader(200) не паникует и не возвращает ошибку (программист об этой проблеме узнаёт только из отдельной строки в логах сервера, если вообще смотрит логи) — он просто игнорируется рантаймом на уровне протокола, но код ПОСЛЕ него (json.NewEncoder(w).Encode(result)) всё равно продолжает выполняться и честно пишет тело — то есть у клиента оказывается статус 400, но тело в формате успешного ответа.

Что случится в проде. Клиент API (мобильное приложение, фронтенд, другой сервис) видит статус 400 и, следуя стандартной логике обработки ошибок, показывает пользователю "ошибка запроса" или ретраит запрос — при этом тело ответа, которое сервер честно отправил, на самом деле содержит корректные данные, которые никто не читает, потому что вся ветка обработки ошибок клиента сработала по статусу.

Баг проявляется именно на "плохих" входных данных, которые в проде встречаются регулярно (например, от бота или устаревшей версии клиента) — то есть не редкий крайний случай, а систематически повторяющееся некорректное поведение именно там, где ожидается предсказуемая обработка ошибки.

Как заметить на ревью. Сигнал — любой if-блок, который вызывает WriteHeader/http.Error/аналог и НЕ заканчивается на return сразу после. Простое эмпирическое правило для ревью любого HTTP-хендлера: после каждого вызова, который пишет статус или тело ответа как реакцию на ошибку, следующая строка обязана быть return — если это не так, стоит спросить "а что выполнится дальше, если мы сюда попали".

Забытый return после первого ответа — то же семейство ошибок, что и забытый return после http.Error в любом хендлере: код формально «отработал», но продолжил выполняться там, где должен был остановиться.

Фрагмент 9

func EnrichOrder(order *Order) error {
    resp, err := http.Get(
        fmt.Sprintf("http://customer-service/customers/%d", order.CustomerID),
    )
    if err != nil {
        return err
    }
    defer resp.Body.Close()

    return json.NewDecoder(resp.Body).Decode(&order.Customer)
}

Баг — забытый таймаут на внешнем вызове. http.Get использует http.DefaultClient без ограничения по времени — если customer-service зависнет (не ответит и не разорвёт соединение), этот вызов будет ждать навсегда. Под нагрузкой это исчерпывает горутины и соединения из пула, постепенно роняя весь вызывающий сервис — самая частая практическая причина каскадных инцидентов в проде.

Механизм. У TCP-соединения нет встроенного таймаута "ждать ответ не дольше X" — если удалённая сторона приняла соединение, но просто ничего не отвечает (зависший процесс, перегруженный воркер, deadlock на её стороне), клиентская сторона будет ждать данные сколько угодно, если явно не указано иное. http.DefaultClient не задаёт Timeout по умолчанию именно поэтому его использование "как есть" без собственного контекста — скрытая ловушка: код выглядит рабочим на любом тестовом окружении, где сервис-зависимость всегда отвечает быстро, и ломается только тогда, когда зависимость реально зависает — то есть именно тогда, когда устойчивость важнее всего.

Что случится в проде. Один зависший customer-service вызывает КАСКАДНЫЙ отказ: каждый запрос к EnrichOrder занимает горутину и открытое соединение навсегда, вместо того чтобы обработаться за миллисекунды. За несколько минут под обычной нагрузкой у вызывающего сервиса заканчиваются свободные горутины/соединения из его собственного пула — и он перестаёт отвечать уже на СВОИ входящие запросы, никак не связанные с обогащением заказа. Классическая картина инцидента "упал один маленький внутренний сервис — легло полполки сервисов сверху", хотя формально каждый из них "просто ждал ответа".

Как заметить на ревью. Сигнал — любой сетевой вызов (HTTP, gRPC, поход в БД/кэш) без контекста с таймаутом, особенно к сервису, который не контролируется той же командой. Стоит проверять на автомате: у функции есть context.Context в сигнатуре? Если нет — как она вообще может быть отменена или ограничена по времени? Если контекст есть, но не факт, что у него уже задан дедлайн выше по цепочке — стоит явно завести свой WithTimeout с разумным значением именно для этого конкретного внешнего вызова.

func EnrichOrder(ctx context.Context, order *Order) error {
    ctx, cancel := context.WithTimeout(ctx, 3*time.Second) // ДОБАВЛЕНО
    defer cancel()

    req, err := http.NewRequestWithContext(ctx, "GET",
        fmt.Sprintf("http://customer-service/customers/%d", order.CustomerID), nil)
    if err != nil {
        return err
    }

    resp, err := http.DefaultClient.Do(req)
    if err != nil {
        return err
    }
    defer resp.Body.Close()

    return json.NewDecoder(resp.Body).Decode(&order.Customer)
}

context.WithTimeout + http.NewRequestWithContext гарантируют, что вызов оборвётся сам не позже отведённого времени, даже если удалённая сторона никогда не ответит — сервис деградирует контролируемо, а не зависает.

Фрагмент 10

func ParseResponse(data []byte) User {
    var u User
    json.Unmarshal(data, u) // без &
    return u
}

Баг — u передан по значению, а не по указателю. json.Unmarshal должен записать результат туда, куда указывает переданный аргумент — для этого нужен адрес. Без & функция получает копию пустой структуры u, заполняет копию, а оригинальная переменная в ParseResponse так и остаётся нулевым значением. Это либо ошибка в рантайме (json: Unmarshal(non-pointer main.User)), либо, того хуже, тихо неверный результат — return u вернёт пустую структуру без единого заполненного поля, без единой ошибки компиляции.

Механизм. Аргументы функций в Go всегда передаются по значению — json.Unmarshal(data, u) передаёт КОПИЮ значения u, а не саму переменную. Функция физически не может изменить переменную вызывающего кода через копию её значения — единственный способ функции "дотянуться" до оригинала снаружи это получить адрес переменной (указатель), после чего писать уже по этому адресу через разыменование.

json.Unmarshal объявлен как func Unmarshal(data []byte, v any) error — принимает any, а не конкретный указательный тип, поэтому компилятор Go НЕ может статически проверить на этапе сборки, что туда передан именно указатель, а не значение: ошибка (если она вообще случится, а не тихо пропустится) обнаружится только в рантайме.

Что случится в проде. Если это, например, код клиента, разбирающего ответ внешнего API, "тихий" сценарий (без явной паники, просто пустая структура) особенно опасен — вызывающий код получит User{} с пустыми полями и, скорее всего, продолжит работать с этим "пользователем" дальше как ни в чём не бывало: запишет пустое имя в БД, отправит письмо на пустой email, залогирует ID равный нулю. Ошибка не остановит выполнение — она молча испортит данные дальше по всей цепочке обработки, и обнаружится только тогда, когда кто-то заметит аномально много "пустых" записей спустя время.

Как заметить на ревью. Сигнал — любой вызов Unmarshal/Scan/аналога, который принимает any/interface{} и должен ЗАПИСАТЬ результат — такие функции по соглашению Go всегда требуют указатель на месте "выходного" параметра. Простое правило: если у функции в описании написано "разбирает/сканирует/декодирует В переданный аргумент" — на этом месте должен быть &переменная, а не голая переменная; при чтении кода на этом месте стоит явно проверить наличие &, это легко пропустить взглядом при беглом просмотре.

func ParseResponse(data []byte) (User, error) {
    var u User
    if err := json.Unmarshal(data, &u); err != nil { // ИСПРАВЛЕНО: &u + проверка ошибки
        return User{}, err
    }
    return u, nil
}

Заодно исправлена вторая, менее заметная проблема оригинала — полностью проигнорированная ошибка Unmarshal: даже с добавленным & сломанный JSON на входе прошёл бы незамеченным.

Фрагмент 11

func FirstTwoWithMarker(items []int) []int {
    base := items[:2]
    return append(base, 999)
}

func main() {
    items := []int{1, 2, 3, 4, 5}
    result := FirstTwoWithMarker(items)
    fmt.Println(result)
    fmt.Println(items) // как думаете, что здесь?
}

Баг — append тихо портит исходный слайс через общий underlying array, реально проверено: items[:2] не копирует данные — это «окно» в тот же массив, что и items, просто с длиной 2 и ёмкостью (cap), унаследованной от исходного слайса (здесь cap=5, есть запас).

append(base, 999) видит, что место в массиве ещё есть (cap позволяет), и пишет 999 прямо в items[2] — туда, где раньше было значение 3. Реальный вывод: result = [1 2 999], а items после вызова — [1 2 999 4 5], хотя FirstTwoWithMarker формально не должна была трогать оригинальный слайс вообще.

Механизм. Срез в Go — это не сами данные, а маленькая структура из трёх полей: указатель на начало данных в массиве, len (сколько элементов видно) и cap (сколько места есть в массиве, начиная с этого указателя, до его конца).

Операция items[:2] не копирует ни одного байта — она создаёт НОВЫЙ заголовок среза с тем же указателем, len=2, и cap, унаследованным от items (в данном случае cap=5, потому что после позиции 2 в массиве ещё есть место под элементы 3,4,5 исходного items). append проверяет только одно условие: помещается ли новый элемент в пределах cap — если да, он пишет ПРЯМО в существующий массив по индексу len, не создавая копию.

Функция FirstTwoWithMarker выглядит как "чистая" (принимает слайс, возвращает новый слайс), но на деле мутирует данные, на которые смотрит аргумент вызывающей стороны, потому что оба слайса — items и base — физически указывают в одну и ту же память.

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

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

Как заметить на ревью. Сигнал — любая функция, которая берёт срез-параметр, делает по нему подсрез (s[:n], s[a:b]) БЕЗ явного ограничения cap (третий индекс) и затем вызывает append на этом подсрезе. Стоит на автомате спрашивать: "у этого подсреза cap совпадает с len, или есть запас?" — если есть запас, append рискует записать поверх данных исходного слайса.

func FirstTwoWithMarker(items []int) []int {    base := items[:2]    return append(base, 999)}

func FirstTwoWithMarker(items []int) []int {    base := items[:2:2] // ИСПРАВЛЕНО: третий индекс среза ограничивает cap    return append(base, 999) // теперь cap(base)==2==len(base) -> append ВЫДЕЛЯЕТ новый массив}

Трёхиндексный срез items[:2:2] (low:high:max) явно ограничивает ёмкость результата, равной его длине — append в этом случае гарантированно не находит свободного места в старом массиве и аллоцирует новый, оставляя items нетронутым. Это одна из самых частых причин «необъяснимой» порчи данных в реальном Go-коде — функция, которая выглядит как чистая (принимает срез, возвращает новый), на деле мутирует аргумент через общую память.

Фрагмент 12

func ProcessFiles(names []string) error {
    for _, name := range names {
        f, err := os.Open(name)
        if err != nil {
            return err
        }
        defer f.Close()

        if err := process(f); err != nil {
            return err
        }
    }
    return nil
}

Баг — defer внутри цикла копится до конца функции, а не до конца итерации, реально проверено с подсчётом открытых файлов: defer выполняется при выходе из функции целиком, не из блока/итерации, в которой был объявлен. При обработке 5 файлов реальный подсчёт показал: после всего цикла, до возврата из ProcessFiles, открытыми оставались все 5 файлов одновременно — ни один не закрылся раньше времени, хотя обработка каждого давно завершилась. На списке из тысяч файлов это реально исчерпывает лимит файловых дескрипторов процесса (too many open files).

Механизм. defer в Go привязан к ФУНКЦИИ, а не к блоку кода (циклу, if, произвольным фигурным скобкам) — это фундаментальное свойство языка, не зависящее от того, где именно текстуально написан defer. Каждый вызов defer f.Close() внутри тела цикла добавляет ОТДЕЛЬНУЮ, независимую запись в стек отложенных вызовов текущей функции; все они выполнятся строго в порядке LIFO (последний добавленный — первый выполненный), но все — только в момент, когда сама функция ProcessFiles реально завершится (через return или панику), а не когда закончится очередная итерация цикла.

Что случится в проде. У операционной системы есть жёсткий лимит на число одновременно открытых файловых дескрипторов на процесс (типично 1024 по умолчанию на Linux, настраивается, но всегда конечен).

Если ProcessFiles вызывается со списком, скажем, из 2000 файлов, лимит будет исчерпан примерно на середине обработки — и все ПОСЛЕДУЮЩИЕ вызовы os.Open начнут возвращать ошибку too many open files, хотя каждый отдельно взятый файл был бы в порядке. Особенно неприятно, что баг не проявляется на малых объёмах (5-10 файлов в тесте — всё работает), поэтому легко проходит код-ревью и юнит-тесты, а всплывает только на реальных боевых данных, где входных файлов бывают тысячи.

Как заметить на ревью. Сигнал — defer любой закрывающей операции (Close, Unlock, cancel() и подобное) внутри тела цикла for, если ресурс создаётся заново на каждой итерации. Правило простое: если ресурс должен жить не дольше одной итерации, defer для его освобождения обязан быть внутри функции, вызов которой ограничен ровно этой итерацией — либо явно вызывать Close() в конце тела цикла без defer, либо (обычно чище) выносить тело цикла в отдельную функцию, как показано в исправлении.

func ProcessFiles(names []string) error {
    for _, name := range names {
        if err := processOne(name); err != nil {
            return err
        }
    }
    return nil
}

func processOne(name string) error {
    f, err := os.Open(name)
    if err != nil {
        return err
    }
    defer f.Close() // теперь это конец processOne - закрывается на каждой итерации, а не в конце всего цикла

    return process(f)
}

Вынесение тела итерации в отдельную функцию — стандартное решение: defer внутри неё срабатывает при возврате из неё, то есть ровно в конце каждой итерации исходного цикла, а не в конце всего ProcessFiles.