Allow for aborts in attempted function

[jbpros]

Within attempted functions, this now exposes an abort() method that accepts an error. Calling it will make attempt stop trying and fail right away.

[jbpros]

My use case for this lies within a repeated find in a database that is expected to fail several times. The record I'm looking for will eventually be available within the next 5 seconds, the not found error is therefore expected within that period and leads to retries.

However, there might be connection issues with the database or other conditions that are considered exceptional and should lead to direct failures.

I've got a similar polling mechanism using attempt in a HTTP-based request.

[TomFrost]

Great use case -- agreed, aborting should definitely be part of Attempt.

I'm not sold on this implementation of it though. Part of the beauty of a good flow control library is that it abstracts away tedium, like nested callbacks and having to assign this to some other variable. This implementation plugs all of that back in, and makes for a bit of a hacky solution.

Attempt solves the above problems by allowing an onError function to be specified. That way, you can write a function that gets to see every error, and you don't need a nested callback to do it. This deserves more thought than I've given it, but a good alternative route might be to support a this.abort() function within the onError call. How would that support your use case?

[TomFrost]

Let me amend the above now that I've thought about it more. One thing I've come to dislike in Attempt is that calling done() from within onError avoids the wait time by default. It should not; in fact, there shouldn't be a mechanism in place to avoid that wait time, because if you're specifying an onError, then you know enough to eliminate the wait time from your options if you don't want it.

If a new major version is released to modify the Attempt API, that issue can be fixed. And if there's no longer a mechanism to avoid the wait from onError, that opens up done() to accept an error argument. If specified, Attempt can immediately abort with that error. Very clean solution. I'd still love to know what you think about that in terms your use case.

[jbpros]

It's interesting you bring up the callback vs. this debate in the conversation, I hesitated to share my thoughts about that. So, here they are:

I'm actually not convinced that using this as the callback holder is a good idea. It really feels counter-intuitive in a Node.js environment, it is not idiomatic and looks more like some jQuery API.

To me and from what we see in 99% of Node APIs, this, in many situations, is about the current object we're running code on/against (most often in an OO context), nothing else. Using it to implicitly pass something that is in reality a callback is kind of cheating, imo.

Here is an fragment of my current code:

attempt(attemptOptions, function (attempts) {
  var callback = this;
  self.collection.findOne(params, function (err, item) {
    if (err) return callback.abort(err);
    if (!item)
      callback(new Error("Report not found (query: " + util.inspect(params) + ")"));
    else
      callback(null, item);
  });
}, function (err, result) {
  // ...
});

See how I have to assign a callback variable anyway because of the closure? I know I could have extracted my stuff into another function, but you don't want to do that in all cases.

That being said, I might be missing the point of the whole library and using it in a twisted way. You tell me :)

[TomFrost]

Definitely a good debate to have. In terms of flow control libraries, using 'this' as the implicit callback was borderline-standard 1-2 years ago when Step and Seq were the leaders in this area. Since then, though, those libraries have lost steam, and the previously-feature-lacking Async library has been enjoying much more widespead implementation. Async.js uses a standard callback argument in each function along the chain, rather than 'this'.

I agree; 'this' is a cheat and a bit of a hack. On the other hand, when 'this' refers to the flow control library itself, having it be something that's also callable doesn't deviate so far from the norm that it's unacceptable. The benefit that's gained is that you always know where the callback is, rather than it being possibly the first argument, or the second, or the third, and so on depending on the function before it. Granted, the Attempt library doesn't have to grapple with this use case, as there are only two functions being passed. I used it initially for clean integration with Step and Seq, so perhaps moving it in the next major release to be more on-par with Async is the best way forward.

With that said, though, your code sample makes me cringe a bit solely because a goal of attempt is to allow for retry functionality without using a nested callback, but you're forced to do that because there's no way to allow onError to abort your trials yet. So regardless of the outcome of the 'this' discussion, I still think moving the abort ability to the onError call to avoid that nested callback structure fits more within the framework of Attempt.