How to prevent disasters caused by the callback getting called multiple times?

[jeevan kk]

I am using different 3rd party modules in my project. I have seen, in some odd situations the 3rd party module which I am using is calling the callback multiple times. 

Is there any general approach which I can follow so avoid such situations. 

[Mark Hahn]

The http module calls back every request.  Sometimes multiple callbacks make sense.  Are the modules you are having trouble with supposed to only return once?  If so then post a bug report. 

I don't know what else you could do.  I guess you could set a flag when you send the callback function, clear it the first callback, and then ignore the callback after that.  I would never do this because I would be blindly ignoring a problem.

On Mon, Sep 30, 2013 at 10:17 PM, jeevan kk <jeev...@gmail.com> wrote:

I am using different 3rd party modules in my project. I have seen, in some odd situations the 3rd party module which I am using is calling the callback multiple times. 

Is there any general approach which I can follow so avoid such situations. 

--
--
Job Board: http://jobs.nodejs.org/
Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
You received this message because you are subscribed to the Google
Groups "nodejs" group.
To post to this group, send email to nod...@googlegroups.com
To unsubscribe from this group, send email to
nodejs+un...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/nodejs?hl=en?hl=en
 
---
You received this message because you are subscribed to the Google Groups "nodejs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to nodejs+un...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[jeevan kk]

Mark,

I agree with you that in some cases its meaningful. But I am into a huge project where we use a large number of third party modules. Its not practical to keep a flag everywhere and check it. I was searching for some general approach which we can follow. We can't find the faulty module until it lands in that edge case. And there are cases where we can ignore the callback getting called multiple times. 

[greelgorke]

are you sure, the callback is called several times? sounds like the a callback is attached several time to me. this can happen in complex environemnt, and you could check that very easy by checking the listeners array of the emitter.

if the array is fine, then you can debug the code. you can inspect the the 3rd party code for something liem ".emit('eventThatCausesTroubles'" and put some console.trace calls here and there. If its a bug a pull request is the best issue message.

[Alex Kocharin]

It's rather simple to create a wrapper that counts a number of times callback is called and throws when this number isn't 1. And call it like this:

fs.readFile('something', wrap(callback));

But you'll have to do it every time, so I don't think it's going to be used widely.

[Adam Crabtree]

Control-flow libraries are the first line of defense between your application code and third-party modules. Stepup for instance, my control-flow library of choice, has such protection: https://github.com/CrabDude/stepup

Cheers,

Adam

--
Better a little with righteousness
       than much gain with injustice.
Proverbs 16:8

[Oleg Slobodskoi]

The answer depends on the reason why the callback called multiple times, possible are:

- by design

- when multiple parallel running functions callback

The last one could happen f.e. one of them fails and calls back with an error, the other one calls back without error despite of the errored other one. In this case you can't ignore either the first one or the second one.

I had this situation often when dealing with event emitters and transforming the results into single callback style.

The only correct way is to fix the function which is causing this error.

Best,
Oleg Slobodskoi

http://www.facebook.com/oleg.slobodskoi
https://www.xing.com/profile/Oleg_Slobodskoi
https://twitter.com/oleg008
https://github.com/kof

Am 01.10.2013 um 07:17 schrieb jeevan kk <jeev...@gmail.com>:

I am using different 3rd party modules in my project. I have seen, in some odd situations the 3rd party module which I am using is calling the callback multiple times. 

Is there any general approach which I can follow so avoid such situations. 

[Mikeal Rogers]

where this is a concern you should just use once. the request 3.0 branch is doing so, as are several other projects i've written, and it works quite well.

https://npmjs.org/package/once

-Mikeal

[Alex Kocharin]

The answer depends on the reason why the callback called multiple times, possible are:

- by design

- when multiple parallel running functions callback

or:

- when there's a missing return statement

like this:
function read_something(name, cb) {
    fs.readFile(name, 'utf8', function(err, result) {
        if (err) cb(err);
        result = foo(bar(result));
        cb(null, result);
    });
});

I'm seeing those bugs much more frequently than I'd like to.


// alex

[Mark Hahn]

> I'm seeing those bugs much more frequently than I'd like to.

Me too.  And I also see the opposite where I don't remember to put cb() everywhere it is needed and no callback happens at all.  At least I know the symptom well.  Everything just stops.

--

[Marco Rogers]

This is the more insidious bug in my mind, because things just hang. Requests randomly time out. It sucks.

[Marco Rogers]

I like this pattern for making nice utilities available on the function prototype. IMO it's the "safest" proto to modify. It would be handy in modules that do other specific function-wrapping things, like memoizing, currying, throttling, etc. This opinion is probably ripe for sparking nerd fights though.

:Marco

[jmar777]

In general, if you're seeing a callback invoked multiple times (when it clearly shouldn't), you're not going to want to mask that problem on your end.  You might get it to where your own callback is only invoked once, but there can be some detrimental side effects on the other side.  That being said, if you determine it's appropriate in your situation, I've got a callback utility that helps with things like callback timeouts, multiple calls, etc.: https://github.com/jmar777/cb

[Felipe Mobus]

Have you looked at promises? Promises will be called only once, and they offer a pretty clean way of doing chains.

My current approach is to either prefer libs that expose promises or, when there is no such option, to wrap calls to the library in a promise.

On 1 Oct 2013 02:17, "jeevan kk" <jeev...@gmail.com> wrote:

I am using different 3rd party modules in my project. I have seen, in some odd situations the 3rd party module which I am using is calling the callback multiple times. 

Is there any general approach which I can follow so avoid such situations. 

--

[Peter Rust]

The async flow-control library (https://github.com/caolan/async/pull/200) goes as far as to throw an error when a callback is called multiple times.

Sure, you could mask the problem via the once library mentioned above or _.once (http://underscorejs.org/#once), but I agree with jmar777 -- instead of masking it, you should treat it seriously and figure out the cause. The few times that this has happened to me, it hasn't been too hard to figure out the cause and fix it -- and I'm glad I did.

[Alexey Petrushin]

var _ = require(underscore)

someFn(_(cb).once())

Also, you can use some metaprogramming wrap third party functions directly

ensureCallbackCalledOnce = function(fn){extract callback from args and wrap it}

someFn = ensureCallbackCalledOnce someFn

someFn(cb)

[Michael Schoonmaker]

+1 for just using the `once` module and being done with it. Measure everything, and worry about the side effects when you see them. If you're already into a "huge project", this is (as always, in my opinion) the most pragmatic option.

--

[jmar777]

+1 for just using the `once` module and being done with it. Measure everything, and worry about the side effects when you see them. If you're already into a "huge project", this is (as always, in my opinion) the most pragmatic option.

Not to be confrontational (but here I go :)), but I have to strongly disagree with this statement. If a callback is being invoked multiple times when it clearly shouldn't be, there is a very real bug in the system that should not be ignored.  Simply using one of these `.once()` utilities is the callback equivalent of:

 

try { doSomething(); } catch (err) { /* who cares? */ }

It makes the immediate problem go away, but at the cost of masking an actual bug.  Even if the immediate side effects aren't significant, ignoring multiple callback invocations puts the system at future risk as the code base evolves. 

[Michael Jackson]

The solution to this problem is very straightforward using promises, assuming you only want the result from the first time the callback is invoked. For example, using the when library (most promise libraries will give you a similar deferred API):

    var when = require('when');

    function doSomethingAsync() {

      var deferred = when.defer();

      misbehavingAsyncApi(function (error, result) {

        if (error) {

          deferred.reject(error);

        } else {

          deferred.resolve(result);

        }

      });

      return deferred.promise;

    }

    doSomethingAsync().then(function (result) {

      // You'll only ever get here once, even if the callback you gave

      // to misbehavingAsyncApi is called multiple times.

    });

Due to the nature of promises, they will only ever be fulfilled once. Callbacks don't give you that same guarantee, which is why they're a pretty poor substitute for a return statement.

Good luck!

--
Michael Jackson
@mjackson

On Mon, Sep 30, 2013 at 10:17 PM, jeevan kk <jeev...@gmail.com> wrote:

I am using different 3rd party modules in my project. I have seen, in some odd situations the 3rd party module which I am using is calling the callback multiple times. 

Is there any general approach which I can follow so avoid such situations. 

--

[Adam Crabtree]

@jmar

It's safe to say that you want both to treat the multiple callbacks as a bug, yet still handle the situation gracefully. In that vein, I agree that using once will only silence the problem in a bad way while protecting your application code. The goal is to have both: a warning / error and protection.

Again, add this manner of protection to your control-flow lib and forget about it. 

Cheers,

Adam

[jeevan kk]

One of the common mistake which I have seen like this.

var connect = function(callback) {

    var b = new Obj();

    b.once('connect', function() {

        callback.apply(null, []);

    });

   

    b.once('error', function(error) {

        callback.apply(null, [error]);

    });

    b.once('timeout', function(error) {

        callback.apply(null, [error]);

    });

};

var execute = function (data, callback) {

    connect(function(err) {

    if(err) {

       callback.apply(null, [err]);

       return;

    }

    //Do something,

    callback.apply(null, []);

    });

} ;

In this situation execute() is the function of the third party module which I am using, and this callback gets called multiple times on some odd situation. Most of the case its like a connect is successfully fired and after that an error is being fired. By the time error is being called I will have successfully completed all my operations. This is just a sample. 

[Mark Hahn]

>  a connect is successfully fired and after that an error is being fired

I don't see why these multiple callbacks are a problem.  They are happening exactly as they are supposed to if they are actually convenience callbacks for event emitters like http.createServer.  You should handle each type of event appropriately.

You seem to have an irrational  fear of multiple callbacks.

--

[jeevan kk]

On Tuesday, 8 October 2013 11:13:24 UTC+5:30, Mark Hahn wrote:

>  a connect is successfully fired and after that an error is being fired

I don't see why these multiple callbacks are a problem.  They are happening exactly as they are supposed to if they are actually convenience callbacks for event emitters like http.createServer.  You should handle each type of event appropriately.

@Mark

Yes I agree to it. But think like its a module to execute a mysql query. So you are using a function execute() to execute a query. As an user why will you expect your callback to be called multiple times? If there is a error it should call it will an error or inform its successfully executed. It shouldn't say the query is successfully executed and later call the callback again with the error.  Doesn't it make sense? Please correct me if I am wrong. I am new to Nodejs  

[Tim Caswell]

On Tue, Oct 8, 2013 at 12:52 AM, jeevan kk <jeev...@gmail.com> wrote:

On Tuesday, 8 October 2013 11:13:24 UTC+5:30, Mark Hahn wrote:

>  a connect is successfully fired and after that an error is being fired

I don't see why these multiple callbacks are a problem.  They are happening exactly as they are supposed to if they are actually convenience callbacks for event emitters like http.createServer.  You should handle each type of event appropriately.

@Mark

Yes I agree to it. But think like its a module to execute a mysql query. So you are using a function execute() to execute a query. As an user why will you expect your callback to be called multiple times? If there is a error it should call it will an error or inform its successfully executed. It shouldn't say the query is successfully executed and later call the callback again with the error.  Doesn't it make sense? Please correct me if I am wrong. I am new to Nodejs  

 

Yes, you're right.  Async functions (the kind that have (err, value) callback as the last arg in node) should call the callback exactly once, either with an error or a value.

I think what Mark is trying to say is that sometimes functions are used as first-class values in cases other than the continuation to some async operation.  If I pass in a function as the event listener for some event that can happen zero or more times, I expect my function to be called zero or more times.  This is natural.  Sometimes functions are used for things that have nothing to do with event loops.  A common example is Array.prototype.forEach.

In all these cases the function being passed in as a value is sometimes called a "callback" which is rather confusing.  For this reason I sometimes avoid the "callback" name.

Going back to your example where an event emitter is wrapped to expose a continuation passing style async function, yes that is a bug if the continuation function gets calls multiple times.  If you can't control the libraries you're using, then some sort of helper like once is not a bad idea.  If you want to be aware of the errors it's suppressing then use a helper that logs them so that you can report errors to the library you're using and hopefully get the issue fixed upstream.