Fixed nasty callback duplication.

[martinheidegger]

I had some case where the second error response was called during the callback of the first error response in an async call before the callback could be replaced with a noop. That resulted in the error case where the callback was called twice with an error message. That was pretty hard to debug. I figured that the same error could occur at other places as well and changed it there too. It would be nice if someone could provide unit tests for these changes, else I will provide them as soon as I have time for it.

[aearly]

I'd really want a failing test case (one that fails without your patch) before we merge this. It feels like you might be doing something wrong.

[caolan]

As @aearly suggests, I think your code prior to this might be calling it's callback multiple times.

Still, this change seems fairly straight-forward and shouldn't cause problems elsewhere. @aearly what do you think about accepting on those grounds?

[aearly]

It is pretty straightforward and harmless, and would catch some of the problems caused by calling-back multiple times. It won't catch everything though, because once you pass a callback to an iterator internally, you can't retroactively change it to a no-op.

It does sort of enable improper async style -- you're only supposed to ever callback once. Always return callback(err, result); etc.... When I'm wrapping a complicated event or timer-driven process in an async function, I always set callback = _.once(callback) first.

If a novice to Node wrote something like:

async.waterfall([
  //...
  function someTask(args, callback) {
    doSomething(args, function (err, result) {
      if (err) {
        callback(err);
      }
      callback(null, foo(result));
    });
  }
  //...
], done);

...they may wonder why someTask works as expected in async.waterfall but not by itself (or in async.series, for that matter). How much hand-holding should async do?

[vsivsi]

Rather than making it more permissive, perhaps async needs a "strict" mode for development/testing/debugging where all callbacks provided to user code are automatically wrapped in the equivalent of _.once(). The best time to catch such issues is as soon as the mistake is introduced, not much later when it ends up impacting something else down the line.

Making async more permissive would simply delay discovering such things until they are much more expensive to find and fix. Introducing a mode where async is super strict would be an aid to learning for novices and would make such bugs much cheaper to find for pros.

In cases such as the OP here, you guys could say "turn on async.strict and get back to us if your code passes...."

[caolan]

@vsivsi let's not make it a mode, let's just make it more strict ;)

@aearly how about we implement the early re-assignment of the callback as in this pull request, but instead of making it a no-op we replace it with a function which throws with a more informative error message? "Callback called more than once"... ?

[martinheidegger]

Let me provide with my error scenario: This is a rough sketch of the way this error bugged me:

https://gist.github.com/martinheidegger/f962d8226d1e4a797f4e

In my case the problem was a lot more tricky because it happened when an exceptions was thrown as a result of the callback triggered. That exception closed the node-domain which triggered the error event and upon the error event it all the remaining connections were closed, resulting in the callback triggering the other methods. Really shitty to debug.

[aearly]

@caolan I was going to suggest that. People might complain about writing to stdout, but it will help catch legitimate errors in async control flow.

@martinheidegger Your example is a bit hard to follow, but i think one of the problems is that finishThisRequest needs to clear the timeout set on line 29. There are 2 ways to exit control flow -- either through the timeout, or by explicitly calling the function (when it is interrupted).

[vsivsi]

let's not make it a mode, let's just make it more strict ;)

I could get behind that!

Also, I just realized that I had a mistaken view of what _.once() does (Not a big underscore.js user), and it is the opposite of what I intended. I was suggesting all single use callbacks provided to user code should be wrapped in:

onceOrDie = (callback) ->
   if async.strict
      alreadyInvoked = false
      return (params...) ->
         if alreadyInvoked
            throw new Error "Single use callback invoked multiple times"
         else
            alreadyInvoked = true
            callback params...
   else
      return callback

That's what I meant by async.strict -- really tough love strict.

So to put this back in the context of this PR: if functionality is changed to make async more permissive (which is how I read this PR) then it becomes more valuable to have a way to turn that off, so that when "you're using it wrong," it becomes blatantly obvious.

However, as @caolan suggests, perhaps the right answer is just to make async stricter overall; which argues against the approach taken by this PR, IMO.

[aearly]

There already is an only_once helper: https://github.com/caolan/async/blob/master/lib/async.js#L27

Perhaps we just need to use this everywhere it applies.

[martinheidegger]

@aearly I changed my example to use clearTimeout but it results in the same error:

https://gist.github.com/martinheidegger/c5a045d0e5ecd8f01e54

I also added some traces to illustrate the problem:

https://gist.github.com/martinheidegger/d4661981a3c7c9b1529b

Output:

Finish called on b
Finished [1] async because some error happened while executing b
Finish called on a
Finished [2] async because the task queue interrupted a
Here the callback is done. [ 2 ]
Here the callback is done. [ 1 ]

The contract of each of requests (a) and (b) is by itself correct. Each callback is called only once. The problem is that the b callback is called before the async callback is finished.

(Note: This example works if I add a nextTick in before interrupting all processes: https://gist.github.com/martinheidegger/4b5a3d25afced38b1b21)

@ALL: I am against making this part "stricter" because yes: this problem could theoretically be solved in my case using a nextTick or setTimeout. However: when you run into this problem it can take a lot of time figuring out why there is a second call on async in the first place. This accidental-synchronous call can be a part of a system error in a second library you use that you can't easily change.

[vsivsi]

@martinheidegger My issue with making libs like async be automatically more forgiving in such cases is that it has strong potential to silently mask the impact of more serious problems.

If async was stricter overall, you'd find out about this kind of situation much sooner. If a more permissive solution turns out to be the best choice for your code, you can always use a callback = _.once(callback) construct to solve the issue as @aearly suggested above. At least you'd be doing it with full knowledge of why it is needed and have an opportunity to consider alternative solutions.

As an aside, I often find that if I'm struggling too hard to make a bit of my own code "just work" with a fairly mature tool like async, I take it as a sign that it's time to step back and reconsider my own logic and/or my choice of tool. 80% of the time I find it's because I've picked the wrong tool for the job, or I'm going about it the wrong way. Not saying that's what's going on with your code, but worth considering.

[martinheidegger]

@vsivsi Async is already forgiving that both requests trigger a "callback" with an error. If you remove the interrupt trigger or move it to a nextTick (out of this method execution scope) then it will work just fine.

It seems to me that this PR has nothing to do with the strictness of async but with consistency within the current strictness. It is correct usage for multiple calls to the callback with a error asynchronously. Why shouldn't it be correct to do the same thing synchronously.

[vsivsi]

I feel the presence of Zalgo...

[martinheidegger]

Zalgo is with us and in us and makes us climb the walls always.

[vsivsi]

@martinheidegger Is it fair to say that this is the simplest "failing case" that you are trying to fix?

var async = require('async');
var shared = null;

async.parallel([
  function(cb) {
    return shared = function() {
      shared = function() {};
      return cb(new Error("Oh, hello!"));
    };
  }, function(cb) {
    return cb(new Error("Bye bye"));
  }
], function(err) {
  console.log("" + err);
  return shared();
});

Outputs:

Error: Bye bye
Error: Oh, hello!

[martinheidegger]

Yes, looks like it.

[vsivsi]

If I introduced logic like this into a program, I'd want to know about it as soon as possible.

By my suggestion above, async would throw an Error instead of invoking the final callback a second time.

If I understand it correctly, this PR would obscure this situation by simply eliminating the second invocation of the user callback. I'm bending my mind trying to imagine a situation where this is a good thing to allow...

Looking at the code in your Gist, it seems to me that the source of the issue is that interruptAllRunningTasks() calls finishThisRequest() for each running task (invoking each task's callback) rather than calling a special abortThisRequest() function that omits invoking the callback (since you know it is being invoked by the error handler of the final callback!)

Fixing it this way requires no use ofsetImmediate(), and doesn't require async to permit this attempted indirect recursive invocation of the final callback.

In my mind, this is a great example of why async should throw an Error in such cases rather than enabling them, so that what is really happening is revealed and logical errors can be caught and fixed, rather than obscured to manifest later in ever more insidious forms. Th͏e Da҉rk Pońy Lo͘r͠d HE ́C͡OM̴E̸S!

[martinheidegger]

@vsivsi The example I gave was pretty simplified, in reality it was more complex. For the past hour I tried to extract the error that occurred but somehow my knowledge of node is not sufficient and I am running a little out of time so... explaining in words:

1. I created a integration test using lab that started two parallel requests to aws using the aws-sdk and I wanted to test if the error cases behaved properly.
2. Because the settings were wrong the first request exited with an error, triggering the async callback.
3. My unit test assumed the error as expected and just finished this current test and moved on to the next.
4. Some event in the unit test system triggered some method in the aws-sdk that told the sdk to finish the second request and since that was an error as well it sent a second callback with an error.
5. The test system sent a error that the current callback has been called already.

Now I ended up with the output that looked something like that:

.x

Error while executing an unrelated test:
   callback has been called twice.

I do not know which exact event triggered Zalgo. But the error message I was left with took me very long to figure out what was actually happening.

Note: I am not 100% sure that this is exactly what happened but that is what I could grasp & remember.

[aearly]

I fixed a lot of these issues recently. In most cases all it took was wrapping the callback with _once. If these issues resurface, please open a new PR with the specifics and a test case.