JavaScript driver - ArrayResult.next is kind of broken

[neumino]

I believe a lot of people expect this code to work (See http://stackoverflow.com/questions/18119387/iterating-over-a-mongodb-cursor-serially-waiting-for-callbacks-before-moving-to/18119789#18119789 or https://github.com/rethinkdb/rethinkdb/blob/next/drivers/javascript/cursor.coffee#L21)

r.connect({port: 28014}, function(err, connection) {
    if (err) throw err;
    r.db("doc").table("users").coerceTo('ARRAY').run( connection, function(err, cursor) {
        if (err) throw err;

        cb = function(err, result) {
            if (err) {
                if (err.message === 'No more rows in the cursor.') {
                    console.log('Done.')
                }
                else {
                    throw err
                }
            }
            else {
                console.log(result)
                cursor.next(cb)
            }
        }

        cursor.next( cb )
    })
})

It throws with

util.js:0
(function (exports, require, module, __filename, __dirname) { // Copyright Joy
^
RangeError: Maximum call stack size exceeded

There are one and a half bugs here.

The first one is that ArrayResult.next doesn't check if there is an element before executing the callback (line 139), so it keeps calling itself with undefined as a result.

The second one is that if the array has more than 3255 elements, we are still going to get a Maximum call stack (node v0.10.20)

While it's technically the user's fault, I think it would be better for us to provide a better implementation of next.I tried a quick fix with setImmediate and it seems to work fine in Node.
setImmediate is not defined in some browsers (including Chrome), but a setTimeout should also do the trick.

I'm not sure, but I think we should also use setImmediate with a normal cursor too. We may hit the same bug when #1543 will be released (or if the user somehow has 4 nested calls in each callback).

[coffeemug]

The second part of the bug should definitely be fixed (for cursors and arrays).

Could you explain what the first bug is more precisely? If there are no more elements, does err get set? (We should fix it if it doesn't -- users should check hasNext or get an error).

[neumino]

When we call .next on ArrayResult, we just call the callback with the value this[this.__proto__.index++] without checking the length of this (this being an array).

So we never send back an RqlDriverError with a "No more rows" error.

[neumino]

Working on this one.

[neumino]

Branch michel_1578_maximum_call_stack
Review 997 assigned to @AtnNn

[neumino]

Review 997
Merged in next as 9077754
Cherry picked in v1.10.x