JSPirates / podcast

Приблизительно еженедельный подкаст о Node.js, модулях, подходах и т.д.
http://jspirates.com
MIT License
35 stars 3 forks source link

Обработка ошибок в коллбеке #13

Closed glukki closed 10 years ago

glukki commented 10 years ago

В 001 выпуске вы упомянули, что обрабатывать в коллбеке ошибку пробросом ошибки на следующий уровень — это плохо (как в примере ниже). А как это делать правильно?

function( err, data ){
  if( err ){
    callback( err );
    return;
  }
  // rocket science
  callback(null, data)
}
sitnin commented 10 years ago

Не совсем так, мы упомянули, что не совсем хорошо делать return.

@dolphin278, расскажем подробности?

sitnin commented 10 years ago

Разобрались. Это был не очень понятный посторонним наш с Борисом давнишний разговор о том, что не стоит мешать синхронный и асинхронный api без надобности.

Однако, мне на ум приходит статья, скорее, стилистического характера о то, что if нужно принципиально всегда делать с секцией else. Автор считал, что это более правильно с точки зрения структурирования кода. Как раз в этом случае отпадает необходимость вызывать return вообще.

Эту статью я читал примерно во время вышеупомянутой беседы с Борисом, поэтому, вероятно, в моей голове все смешалось и переварилось таким вот образом.

В моей версии, приведённый вами код был бы таким:

function (err, data) {
  if (err) {
    callback(err);
  } else {
    // rocket science
    callback(null, data)
  }
}
glukki commented 10 years ago

О, теперь понятно! Ну, а я со времен работы с питоном стараюсь руководствоваться правилом «return early». Например, если бы требовалось сделать ручное кеширование файлов в памяти, я сделал бы это примерно так :)

var fs = require( 'fs' );
var cache = {};

var path = fs.realpathSync( './files/' ) + '/';

function getFile( name, callback ) {
    if ( !callback ) {
        throw ( new Error( 'No callback' ) );
    }

    if ( !name ) {
        callback( new Error( 'No name' ) );
        return;
    }

    if ( cache[name] !== undefined ) {
        callback( null, cache[name] );
        return;
    }

    fs.realpath( (path + name), function( err, filepath ) {
        if ( err ) {
            callback( err );
            return;
        }

        if ( !filepath.contains( path, 0 ) ) {
            callback( new Error( 'Out of dir' ) );
            return;
        }

        fs.readFile( filepath, function( err, file ) {
            if ( err ) {
                callback( err );
                return;
            }

            cache[name] = file;
            callback( null, file );
        } );
    } );
}
sitnin commented 10 years ago

Это ведь, в общем-то, семантический способ избежать проблем, поэтому в питоне, где нет блочных скобок (которые фигурные в js) подход return early полностью оправдан и соответствует семантике языка.

В языках же, в которых есть оператор блока, эту же задачу, как раз, решает правило полного if-а.

sitnin commented 10 years ago

Полный if фигов тем, что он однозначно повышает степень вложенности, которая в js-е растёт, как на дрожжах при первом удобном случае. Поэтому, очевидно, выбор применять или нет такое правило -- дело принятых соглашений по стилю и личного вкуса.

glukki commented 10 years ago

О, и правда, в питоне — способ избежать проблем. Не задумывался об этой стороне вопроса, спасибо :) Теперь, думаю, можно закрывать

dolphin278 commented 10 years ago

Собственно, две копейки по поводу бед от смешивания синхронного и асинхронного — http://nodejs.org/api/all.html#all_process_nexttick_callback

return cb(err);, очевидно, делает вызов мгновенно, и если в функции, которая вызывала вашу функцию, что-то предполагало, что callback будет вызван не раньше, чем она полностью дойдет до конца, то это что-то ждет жестокое разочарование, а разработчика — долгие увлекательные минуты отладки.

Чтобы не распихивать все через setImmediate(), который может здорово замедлить обработку, если система находится под активным IO, можно использовать старый добрый nextTick(), который а) будет выполнен до IO, и до 1000 таких за тик, б) будет выполнен гарантированно после того, как тело вызывающей функции закончится.

:dolphin:

glukki commented 10 years ago

О, б-же! Такое простое, и такое неочевидное! :(

Значит, меня спасало лишь то, что вызов асинхронной функции всегда стоял последним. А решение о запуске нескольких параллельно принимал async.auto, например. Видимо, в нем тоже асинхронный вызов идет последним - надо проверить.

Большое спасибо, пойду исправлять.

dolphin278 commented 10 years ago

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

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