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

Первая половина: фрагменты 1-6, от утечки горутины до забытого resp.Body.Close(). Каждый факт уже проверен реальным go run/go vet/-race при первом написании этого материала — цифры и вывод не выдуманы.

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

Фрагмент 1

func StartWorker(results chan<- int) {
    go func() {
        v := computeExpensive()
        results <- v   // блокирующая запись, без select на ctx.Done()
    }()
}

func main() {
    ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
    defer cancel()

    results := make(chan int) // небуферизированный
    StartWorker(results)

    select {
    case v := <-results:
        fmt.Println(v)
    case <-ctx.Done():
        fmt.Println("таймаут, дальше не ждём")
        // ФУНКЦИЯ main ЗАВЕРШАЕТСЯ, но воркер продолжает жить
    }
}

Баг — утечка горутины: если computeExpensive() выполняется дольше 100мс, main выбирает ветку ctx.Done() и завершается. Горутина внутри StartWorker при этом не узнаёт об отмене — она блокируется на results <- v навсегда, потому что канал небуферизированный, а читатель (main) уже ушёл. Утечка горутины: она держит стек и захваченные ресурсы, пока процесс жив.

Почему это происходит на уровне рантайма. Отправка в небуферизированный канал — это rendezvous: горутина-отправитель физически не может продолжить выполнение, пока какая-то другая горутина не выполнит соответствующий приём. Планировщик Go не считает такую заблокированную горутину "мёртвой" или "зависшей с ошибкой" — с его точки зрения она просто ждёт своей очереди, и будет ждать сколько угодно, потому что нет никакого встроенного таймаута на операции с каналами. Горутина не мусор, который соберёт GC, — заблокированная горутина держит свой стек и всё, что она захватила по замыканию, до конца жизни процесса.

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

Сервис не падает сразу: рост потребления памяти постепенный, метрика числа горутин (runtime.NumGoroutine()) на графике монотонно растёт часами или днями, а потом резко приходит OOM-килл, обычно ночью, без явной корреляции с текущей нагрузкой — потому что причина не в текущем трафике, а в накопленном за предыдущие часы.

Как заметить на ревью. Сигнал — любая горутина, которая делает блокирующую операцию с каналом (ch <- v или <-ch) БЕЗ соседнего select с ctx.Done()/case для отмены, особенно если у вызывающей функции контекст с таймаутом уже есть в сигнатуре, но не передан внутрь горутины. Стоит на автомате задавать себе вопрос "а что, если читатель этого канала уже ушёл к моменту, когда сюда дойдёт исполнение" — если ответ "тогда мы зависнем навсегда", это и есть баг.

func StartWorker(ctx context.Context, results chan<- int) {
    go func() {
        v := computeExpensive()
        select {
        case results <- v:
        case <-ctx.Done(): // ДОБАВЛЕНО: не блокируемся навсегда, если читателя больше нет
        }
    }()
}

Исправление — передать тот же ctx внутрь горутины и обернуть отправку в select с ctx.Done(), а не полагаться на голую блокирующую запись в канал.

Фрагмент 2

type Account struct {
    mu      sync.Mutex
    Balance int
}

func Transfer(a, b *Account, amount int) {
    a.mu.Lock()
    defer a.mu.Unlock()

    b.mu.Lock()
    defer b.mu.Unlock()

    a.Balance -= amount
    b.Balance += amount
}

// Вызывается конкурентно из разных горутин:
// go Transfer(acc1, acc2, 100)
// go Transfer(acc2, acc1, 50)

Баг — классический ABBA-deadlock, реально воспроизведён: горутина 1 захватывает acc1.mu, затем пытается захватить acc2.mu. Одновременно горутина 2 захватывает acc2.mu, затем пытается захватить acc1.mu. Обе держат один лок и ждут другой — взаимная блокировка навсегда. Реальный прогон этого кода: обе транзакции зависают, ни одна не завершается, даже с 2-секундным таймаутом ожидания.

Механизм на уровне рантайма. Mutex.Lock() — блокирующий вызов: горутина физически не продолжит исполнение, пока лок не освободится. Если горутина 1 уснула на acc2.mu.Lock(), удерживая при этом acc1.mu, а горутина 2 в этот же момент уснула на acc1.mu.Lock(), удерживая acc2.mu — оба лока никогда не освободятся, потому что освободить их могут только defer Unlock(), которые сработают лишь при завершении функции, а функции не завершатся, пока не получат встречный лок.

Это не гонка данных (никто ничего не портит) и не временная задержка — это структурный тупик, который сохранится буквально навсегда, сколько бы ни ждали. go run -race его НЕ найдёт: race detector ищет неупорядоченный доступ к памяти, а не циклы ожидания блокировок.

Что случится в проде. В реальном сервисе денежных переводов это выглядит не как явная ошибка, а как "зависшие" запросы: HTTP-хендлер, вызвавший Transfer, никогда не получит ответ и будет висеть до истечения серверного таймаута (если он вообще есть).

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

Как заметить на ревью. Сигнал — функция, которая захватывает ДВА ИЛИ БОЛЕЕ мьютекса, принадлежащих разным объектам, переданным как аргументы. Ключевой вопрос: "а что, если эту же функцию вызовут ещё раз с теми же двумя объектами, но переставленными местами, конкурентно с первым вызовом?"

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

func Transfer(a, b *Account, amount int) {
    // ИСПРАВЛЕНО: всегда захватываем локи в ОДНОМ порядке (по адресу),
    // независимо от того, кто "a", а кто "b" в конкретном вызове
    first, second := a, b
    if uintptr(unsafe.Pointer(a)) > uintptr(unsafe.Pointer(b)) {
        first, second = b, a
    }
    first.mu.Lock()
    defer first.mu.Unlock()
    second.mu.Lock()
    defer second.mu.Unlock()

    a.Balance -= amount
    b.Balance += amount
}

Идея та же, что и сортировка item'ов по ID перед списанием stock в реальном REST-сервисе — фиксированный глобальный порядок захвата локов делает deadlock невозможным в принципе, а не менее вероятным.

Фрагмент 3

func ComputeAll(inputs []int) []int {
    var results []int
    var wg sync.WaitGroup

    for _, n := range inputs {
        wg.Add(1)
        go func(n int) {
            defer wg.Done()
            results = append(results, n*n)
        }(n)
    }
    wg.Wait()
    return results
}

Баг — race condition на append, реально воспроизведён: append не атомарен — читает текущую длину/ёмкость слайса, возможно копирует в новый массив, пишет указатель обратно. 100 конкурентных горутин делают это без синхронизации. Реальный прогон дал len(results) == 26 вместо ожидаемых 100 — часть записей молча потерялась (не паника, просто неверный результат). С go run -race детектор ловит гонку явно.

Механизм поломки. append(results, x) на самом деле три отдельных шага: прочитать текущие len/cap/указатель results, вычислить новое значение (записать x в ячейку len, увеличив len на 1, реаллоцировав массив при нехватке cap), и записать обновлённый заголовок слайса обратно в переменную results.

Когда две горутины делают это одновременно без синхронизации, обе могут прочитать ОДИН И ТОТ ЖЕ старый заголовок results (например, len=5), обе вычислить свою версию с len=6, и одна из этих записей просто перезапишет другую — значение, которое горутина Б только что дописала, теряется, потому что горутина А следом записывает заголовок, как будто Б вообще не работала. Ключевая деталь: программа не падает и не подаёт никакого сигнала об ошибке — она просто молча возвращает неполный результат, который выглядит совершенно правдоподобно (валидный слайс, просто короче, чем должен быть).

Что случится в проде. Если это часть, например, батч-обработки заказов или агрегации метрик по N источникам, часть данных будет систематически и незаметно теряться — не с явной ошибкой в логах, а просто как "почему-то сумма не сходится" или "часть записей потерялась" через недели после деплоя, когда кто-то заметит расхождение с ожидаемым количеством.

Это одна из самых коварных категорий багов именно потому, что она не бросается в глаза сразу: тесты на маленьком количестве горутин (2-3) могут проходить месяцами, потому что вероятность реальной коллизии по времени низкая, а на 100+ горутинах под нагрузкой она становится почти гарантированной.

Как заметить на ревью. Сигнал — append к переменной, объявленной ВНЕ горутины и используемой ВНУТРИ нескольких горутин, без мьютекса или канала вокруг записи. Стоит сразу спросить: "сколько горутин одновременно могут вызвать эту строку, и что произойдёт, если это случится буквально в одну наносекунду друг за другом?" Отдельно: go vet эту конкретную ошибку не ловит (это не про мьютекс по значению), поэтому единственный надёжный автоматический способ — реально прогнать код с -race хотя бы раз в CI, глазами такую гонку заметить можно, но полагаться только на код-ревью без race-детектора рискованно.

func ComputeAll(inputs []int) []int {
    results := make([]int, len(inputs)) // предвыделено, каждая горутина пишет в СВОЙ индекс
    var wg sync.WaitGroup

    for i, n := range inputs {
        wg.Add(1)
        go func(i, n int) {
            defer wg.Done()
            results[i] = n * n // запись по индексу - нет общего состояния для гонки
        }(i, n)
    }
    wg.Wait()
    return results
}

Раз индексы результатов не пересекаются между горутинами, синхронизация вообще не нужна — предвыделенный слайс с записью по своему индексу устраняет саму возможность гонки, а не просто защищает её мьютексом.

Фрагмент 4

func GetUserProfiles(db *sql.DB) ([]Profile, error) {
    rows, err := db.Query("SELECT id, name FROM users")
    if err != nil {
        return nil, err
    }
    defer rows.Close()

    var profiles []Profile
    for rows.Next() {
        var u User
        rows.Scan(&u.ID, &u.Name)

        var bio string
        db.QueryRow("SELECT bio FROM profiles WHERE user_id = $1", u.ID).Scan(&bio)

        profiles = append(profiles, Profile{User: u, Bio: bio})
    }
    return profiles, nil
}

Баг — классическая проблема N+1: один запрос за списком из N пользователей, затем ещё N отдельных запросов внутри цикла — по одному на каждого пользователя. На 3 пользователях в тесте разница незаметна, на 10 000 в проде — 10 001 запрос к базе вместо 2, каждый со своим сетевым round-trip.

Почему это вообще происходит. Код написан так, как естественно думает человек, решающий задачу пошагово: "сначала получу список пользователей, а для каждого — отдельно его bio". Технически каждая такая мысль превращается в отдельный db.QueryRow внутри цикла for rows.Next(), и компилятор/рантайм никак не мешают этому — с точки зрения синтаксиса Go это совершенно валидный, читаемый код, никаких предупреждений при сборке.

Проблема не в синтаксисе, а в том, что каждый QueryRow — это отдельный сетевой round-trip до базы данных (соединение уже установлено, но всё равно есть задержка на передачу запроса и ответа туда-обратно), и она никак не видна, глядя на код построчно, только если явно посчитать, сколько раз выполнится тело цикла.

Что случится в проде на реальных числах. При задержке сети до БД даже в 2мс (оптимистичный сценарий внутри одного дата-центра) 10 000 дополнительных round-trip'ов — это дополнительные ~20 секунд суммарного времени ожидания сети на один вызов GetUserProfiles, даже если сама база отвечает мгновенно. На проде это часто выглядит не как явная ошибка, а как "эта ручка почему-то медленная и не масштабируется" — на 10 пользователях в стейджинге всё летает, а с ростом продакшен-данных время ответа растёт линейно вместе с количеством строк, и однажды упирается в таймаут запроса.

Как заметить на ревью. Сигнал — любой SQL-запрос (db.Query/db.QueryRow/ORM-вызов) ВНУТРИ цикла, который сам итерируется по результату другого запроса. Стоит сразу задать вопрос "а можно ли это выразить одним запросом с JOIN или через WHERE id IN (...)", даже если объём тестовых данных сейчас маленький и разницы не видно — N+1 часто проходит код-ревью именно потому, что на dev-окружении с горсткой строк никакой деградации не заметно, а вылезает только с ростом реальных данных.

func GetUserProfiles(db *sql.DB) ([]Profile, error) {
    // ОДИН JOIN вместо N дополнительных запросов
    rows, err := db.Query(`
        SELECT u.id, u.name, COALESCE(p.bio, '')
        FROM users u LEFT JOIN profiles p ON p.user_id = u.id
    `)
    if err != nil {
        return nil, err
    }
    defer rows.Close()

    var profiles []Profile
    for rows.Next() {
        var pr Profile
        if err := rows.Scan(&pr.User.ID, &pr.User.Name, &pr.Bio); err != nil {
            return nil, err
        }
        profiles = append(profiles, pr)
    }
    return profiles, rows.Err()
}

LEFT JOIN (не INNER) — намеренно, чтобы пользователи без профиля тоже попали в результат. Второй, менее очевидный баг в оригинале — обе ошибки rows.Scan и db.QueryRow(...).Scan внутри цикла проигнорированы (нет проверки err) — молчаливая порча данных при сбое запроса.

Фрагмент 5

func DecrementStock(db *sql.DB, itemID int, qty int) error {
    var stock int
    err := db.QueryRow(
        "SELECT stock FROM items WHERE id = $1", itemID,
    ).Scan(&stock)
    if err != nil {
        return err
    }

    if stock < qty {
        return errors.New("insufficient stock")
    }

    _, err = db.Exec(
        "UPDATE items SET stock = stock - $1 WHERE id = $2", qty, itemID,
    )
    return err
}

Баг — race condition «прочитать-проверить-обновить» (check-then-act): между SELECT stock и UPDATE проходит время. Две конкурентные транзакции могут обе прочитать stock = 1, обе решить, что товара хватает на списание 1 штуки, и обе выполнить UPDATE — итоговый остаток уйдёт в -1, хотя каждая проверка по отдельности была «корректной». Без блокировки строки это два отдельных SQL-запроса, не одна атомарная операция.

Механизм на уровне БД. Дефолтный уровень изоляции Postgres (READ COMMITTED) не спасает здесь, даже если формально нет ни одной классической аномалии чтения (dirty/non-repeatable read) — проблема не в том, что кто-то видит "неправильные" данные, а в том, что между двумя отдельными операциями (SELECT, потом отдельно UPDATE) физически ничего не мешает второй транзакции сделать то же самое. Каждая транзакция честно видит актуальный на момент своего SELECT остаток — просто ничего не связывает этот момент чтения с моментом записи в ту же транзакцию, и вторая параллельная транзакция легко втискивается между ними.

Что случится в проде. Это классический overselling: интернет-магазин продаёт последнюю единицу товара двум покупателям одновременно (высокая нагрузка в момент дропа популярного товара — ровно тот сценарий, когда гонка становится вероятной, а не редкой). Результат — либо отрицательный остаток в БД (если constraint не настроен), либо честная ошибка constraint'а уже ПОСЛЕ того, как оба заказа формально приняты и подтверждены клиентам, что оборачивается ручной отменой одного заказа, возвратом денег и недовольным покупателем — заметно дороже, чем просто вернуть "нет в наличии" на секунду позже.

Как заметить на ревью. Сигнал — любая пара "прочитать значение → в коде приложения решить, что с ним делать → отдельным запросом записать результат этого решения" применительно к общему ресурсу с ограниченным количеством (остаток, баланс, число мест). Правильный вопрос на ревью: "что если между этими двумя запросами проскочит точно такой же параллельный вызов — что тогда увидит вторая транзакция и что она сделает?"

Если ответ — "то же самое, что первая, и они обе применят свою запись" — нужна либо атомарная условная операция уровня БД (как в исправлении ниже), либо явная блокировка строки (SELECT ... FOR UPDATE).

func DecrementStock(db *sql.DB, itemID int, qty int) error {
    // ИСПРАВЛЕНО: одна атомарная условная UPDATE вместо SELECT+UPDATE
    tag, err := db.Exec(
        "UPDATE items SET stock = stock - $1 WHERE id = $2 AND stock >= $1",
        qty, itemID,
    )
    if err != nil {
        return err
    }
    if n, _ := tag.RowsAffected(); n == 0 {
        return errors.New("insufficient stock")
    }
    return nil
}

СУБД сама атомарно проверяет условие stock >= qty и делает вычитание в рамках одной операции — конкурентная вторая транзакция либо не найдёт подходящую строку (если первая уже списала последний товар), либо честно выполнится после первой. RowsAffected() == 0 — способ узнать, что условие не выполнилось, без отдельного чтения.

Фрагмент 6

func FetchUser(id int) (*User, error) {    resp, err := http.Get(fmt.Sprintf("https://api.example.com/users/%d", id))    if err != nil {        return nil, err    }    if resp.StatusCode != http.StatusOK {        return nil, fmt.Errorf("unexpected status: %d", resp.StatusCode)    }    var u User    if err := json.NewDecoder(resp.Body).Decode(&u); err != nil {        return nil, err    }    return &u, nil}

func FetchUser(id int) (*User, error) {    resp, err := http.Get(fmt.Sprintf("https://api.example.com/users/%d", id))    if err != nil {        return nil, err    }    defer resp.Body.Close() // ДОБАВЛЕНО: сразу после успешного получения resp, до любых return    if resp.StatusCode != http.StatusOK {        return nil, fmt.Errorf("unexpected status: %d", resp.StatusCode)    }    var u User    if err := json.NewDecoder(resp.Body).Decode(&u); err != nil {        return nil, err    }    return &u, nil}

Баг — resp.Body никогда не закрывается. Стандартная библиотека Go документирует прямую обязанность вызывающего кода закрыть тело ответа, чтобы соединение вернулось в пул для переиспользования (тот же принцип, что rows.Close() для БД) — забытый Close по каждому пути выхода из функции, включая ранний return при ошибке статуса, постепенно исчерпывает пул TCP-соединений под нагрузкой.

Механизм на уровне транспорта. http.Client по умолчанию держит пул переиспользуемых TCP-соединений (Transport с keep-alive) именно для того, чтобы не устанавливать новое TCP+TLS-соединение на каждый запрос — это дорого по времени.

Соединение возвращается в пул для переиспользования только после того, как тело ответа полностью прочитано И закрыто; если resp.Body не закрыт, соединение считается всё ещё "занятым" этим запросом и не может быть переиспользовано, даже если сам код давно вышел из функции и забыл о нём. В примере выше это особенно коварно: путь, где resp.StatusCode != http.StatusOK, возвращает ошибку раньше, чем код доходит до чтения тела — то есть баг срабатывает именно на ошибочных ответах, которые как раз чаще всего плохо покрыты тестами.

Что случится в проде. У Transport есть ограничение на максимальное число соединений на хост (MaxIdleConnsPerHost, по умолчанию довольно скромное).

Если сервис регулярно ходит к внешнему API, который иногда отвечает не-200 статусами, каждый такой ответ "съедает" одно соединение навсегда. Через какое-то время под нагрузкой пул исчерпывается, и НОВЫЕ запросы к тому же хосту начинают либо ждать освобождения соединения, либо получать ошибку — то есть сервис, обращающийся к абсолютно рабочему внешнему API, начинает деградировать сам по себе, без какой-либо видимой внешней причины, просто потому, что накопил утечку соединений на предыдущих ошибочных ответах.

Как заметить на ревью. Сигнал — любой http.Get/http.Client.Do/аналог, после которого нет defer resp.Body.Close() СРАЗУ после проверки ошибки самого запроса (не после проверки статус-кода — это будет уже поздно для путей с ранним return). Хорошее эмпирическое правило: между строкой, где появилась переменная resp, и следующей проверкой if не должно быть ничего, кроме defer resp.Body.Close().

defer сразу после проверки err от http.Get гарантирует закрытие при любом дальнейшем пути выхода из функции — не нужно помнить закрыть его в каждой отдельной ветке return.