Callback called multiple times on error

[gabmontes]

If the callback to a find().toArray(cb) query throws an error, it is called multiple times. For instance, executing the following code in test.js:

var Db = require('tingodb')().Db;
var db = new Db('./db', {});
var collection = db.collection('some_collection');
collection.insert([{ hello: 'world' }], { w: 1 }, function () {
  collection.find({ hello: 'world' }).toArray(function () {
    console.log('Callback called!')
    throw new Error('Some error');
  });
});

results in the following:

$ node test.js
Callback called!
Callback called!
Callback called!
Callback called!
/<path_to>/test.js:9
    throw new Error('Some error');
    ^
<stack_trace>
$

It is possible that the issue is not only related to toArray() but others. I had no changes to test it further. Tested on v0.3.4 and v0.4.0.

[jamilservicos]

try:

collection.insert([{ hello: 'world' }], { w: 1 }, function () {
    collection.find({ hello: 'world' }).toArray(function (e, a) {
        if(e) { throw new Error(e); }
        console.log(a);
    });
});

[sergeyksv]

In NodeJS and Golang world you should check error after each function call. So I do believe that you has issue on insert and you not check error and continue.

collection.insert([{ hello: 'world' }], { w: 1 }, function (err,) {
  if (err) doSomethingAndNotContinue
  collection.find({ hello: 'world' }).toArray(function (err) {
    if (err) doSomethingAndNotContinue

Our way to handle this boring things is https://github.com/sergeyksv/safejs. For sure you can find other approaches.

[gabmontes]

@jamilservicos my example was oversimplified to show up the issue: the callback function is called more than once and that is not an expected behavior.

@sergeyksv thanks for your suggestion. Anyway, as a user of the library, I expect the callback to be called only once. I should not, as a user, write code that prevents my code to break when the underlying library do strange things.

[sergeyksv]

As a user you should not continue following operation if previous fail. I.e. if you try top open door and it gets failed to open you didn't come through closed door, right? If on your own will you will go through closed door it will be strange to complain about amount of bruises that you will get.

So you need to catch every error and not continue. Plus you first error that you handle should give you some clue about what is goes wrong. Both command should not fail in normal conditions, so you clearly have some global issue and in first order you have to check what it is.

Besides that, and probably this is the cause for this case, throw new Error("something") it not normal for Javascript. Who you suppose to handle that error? throw work in scope of current function, so if you throw in scope of callback error will bubble up to database code which will handle it in some way that I simply cannot predict. Again take a look to safejs and to the code that is correct with error handling:

 function (callback) {
        try {
            // .. do something that can throw error
            // .. note ANY CODE CAN DO in some conditions
            async.forEach(array, function (e, callback2) {
                some.getSome(e.id, function (err, some) {
                    try {
                        if (err) return callback2(err);
                        // .. process some
                        callback2();
                    } catch (err) {
                        callback2(err);
                    }
                })
            },callback)
        } catch (err) {
            callback(err);
        }
    }

[gabmontes]

@sergeyksv as previously stated, the code I initially posted was oversimplified to show the behavior. Take a look at this other code example:

var Db = require('tingodb')().Db;
var db = new Db('./db', {});
var collection = db.collection('some_collection');
collection.insert([{ hello: 'world' }], { w: 1 }, function (err) {
  if (err) {
    console.log('Insert command error');
    return;
  }
  collection.find({ hello: 'world' }).toArray(function (err, array) {
    if (err) {
      console.log('Find command error');
      return;
    }
    console.log('Records found');
    console.log(array[0].inexistent.property) // force an error to be thrown just to show the issue
  });
});

The results of running it will be:

$ node test.js 
Records found
Find command error

So, if something goes wrong during the processing as shown above, the callback given to find() will be called more than once, which is not expected. The lib shall call that callback only once no matter what the user does in there.

[besworks]

@gabmontes Thank you for pointing out this behavior. I had been simply ignoring errors generated by toArray and moving on to the next operation which acts as a fallback if no results are found. I was doing some refactoring and suddenly the callback to toArray started being executed twice. I couldn't for the life of me understand why this was happening until I read your last comment. Now it makes sense, I had changed a variable name inside the callback but had forgotten to change one reference and toArray was catching my ReferenceError. You helped solved my issue but I agree that this is definitely not how I expected it to work. I had assumed that my error handling code would only catch errors directly related to the toArray method and that any other errors in the callback itself would still throw an exception.

@sergeyksv It doesn't really seem appropriate to use safejs for executing user defined callbacks. Besides the fact that invoking the callback twice is just bizarre, toArray (or any other method) should not care what my callback does with the results. If I have operations that need to fail gracefully then I will use my own try...catch block and if I produce an unhandled exception then I would expect it to be thrown immediately so that I can find and correct the issue. By the way, this is exactly how node-mongodb-native handles this scenario when invoking a callback.