Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Short circuit loops completely for some,every,detect when ready #1306

Merged
merged 2 commits into from Oct 16, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/internal/breakLoop.js
@@ -0,0 +1,3 @@
// A temporary value used to identify if the loop should be broken.
// See #1064, #1293
export default {};
27 changes: 12 additions & 15 deletions lib/internal/createTester.js
@@ -1,30 +1,27 @@
'use strict';
import noop from 'lodash/noop';
import breakLoop from './breakLoop';

export default function _createTester(eachfn, check, getResult) {
return function(arr, limit, iteratee, cb) {
function done(err) {
function done() {
if (cb) {
if (err) {
cb(err);
} else {
cb(null, getResult(false));
}
cb(null, getResult(false));
}
}
function wrappedIteratee(x, _, callback) {
if (!cb) return callback();
iteratee(x, function (err, v) {
if (cb) {
if (err) {
cb(err);
cb = iteratee = false;
} else if (check(v)) {
cb(null, getResult(true, x));
cb = iteratee = false;
}
// Check cb as another iteratee may have resolved with a
// value or error since we started this iteratee
if (cb && (err || check(v))) {
if (err) cb(err);
else cb(err, getResult(true, x));
cb = iteratee = false;
callback(err, breakLoop);
} else {
callback();
}
callback();
});
}
if (arguments.length > 3) {
Expand Down
7 changes: 5 additions & 2 deletions lib/internal/eachOfLimit.js
Expand Up @@ -4,6 +4,8 @@ import once from './once';
import iterator from './iterator';
import onlyOnce from './onlyOnce';

import breakLoop from './breakLoop';

export default function _eachOfLimit(limit) {
return function (obj, iteratee, callback) {
callback = once(callback || noop);
Expand All @@ -14,13 +16,14 @@ export default function _eachOfLimit(limit) {
var done = false;
var running = 0;

function iterateeCallback(err) {
function iterateeCallback(err, value) {
running -= 1;
if (err) {
done = true;
callback(err);
}
else if (done && running <= 0) {
else if (value === breakLoop || (done && running <= 0)) {
done = true;
return callback(null);
}
else {
Expand Down
26 changes: 26 additions & 0 deletions mocha_test/detect.js
@@ -1,5 +1,6 @@
var async = require('../lib');
var expect = require('chai').expect;
var _ = require('lodash');

describe("detect", function () {

Expand Down Expand Up @@ -134,6 +135,31 @@ describe("detect", function () {
});
});

it('detectSeries doesn\'t cause stack overflow (#1293)', function(done) {
var arr = _.range(10000);
let calls = 0;
async.detectSeries(arr, function(data, cb) {
calls += 1;
async.setImmediate(_.partial(cb, null, true));
}, function(err) {
expect(err).to.equal(null);
expect(calls).to.equal(1);
done();
});
});

it('detectLimit doesn\'t cause stack overflow (#1293)', function(done) {
var arr = _.range(10000);
let calls = 0;
async.detectLimit(arr, 100, function(data, cb) {
calls += 1;
async.setImmediate(_.partial(cb, null, true));
}, function(err) {
expect(err).to.equal(null);
expect(calls).to.equal(100);
done();
});
});

it('find alias', function(){
expect(async.find).to.equal(async.detect);
Expand Down
27 changes: 27 additions & 0 deletions mocha_test/every.js
@@ -1,5 +1,6 @@
var async = require('../lib');
var expect = require('chai').expect;
var _ = require('lodash');

describe("every", function () {

Expand Down Expand Up @@ -83,6 +84,32 @@ describe("every", function () {
});
});

it('everySeries doesn\'t cause stack overflow (#1293)', function(done) {
var arr = _.range(10000);
let calls = 0;
async.everySeries(arr, function(data, cb) {
calls += 1;
async.setImmediate(_.partial(cb, null, false));
}, function(err) {
expect(err).to.equal(null);
expect(calls).to.equal(1);
done();
});
});

it('everyLimit doesn\'t cause stack overflow (#1293)', function(done) {
var arr = _.range(10000);
let calls = 0;
async.everyLimit(arr, 100, function(data, cb) {
calls += 1;
async.setImmediate(_.partial(cb, null, false));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, setImmediate will partially apply params for you, no need for _.partial

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought our implementation didn't (though didn't check :))

}, function(err) {
expect(err).to.equal(null);
expect(calls).to.equal(100);
done();
});
});

it('all alias', function(){
expect(async.all).to.equal(async.every);
});
Expand Down
29 changes: 28 additions & 1 deletion mocha_test/some.js
@@ -1,5 +1,6 @@
var async = require('../lib');
var expect = require('chai').expect;
var _ = require('lodash');

describe("some", function () {

Expand Down Expand Up @@ -83,7 +84,6 @@ describe("some", function () {
});
});


it('someLimit short-circuit', function(done){
var calls = 0;
async.someLimit([3,1,2], 1, function(x, callback){
Expand All @@ -97,6 +97,33 @@ describe("some", function () {
});
});


it('someSeries doesn\'t cause stack overflow (#1293)', function(done) {
var arr = _.range(10000);
let calls = 0;
async.someSeries(arr, function(data, cb) {
calls += 1;
async.setImmediate(_.partial(cb, null, true));
}, function(err) {
expect(err).to.equal(null);
expect(calls).to.equal(1);
done();
});
});

it('someLimit doesn\'t cause stack overflow (#1293)', function(done) {
var arr = _.range(10000);
let calls = 0;
async.someLimit(arr, 100, function(data, cb) {
calls += 1;
async.setImmediate(_.partial(cb, null, true));
}, function(err) {
expect(err).to.equal(null);
expect(calls).to.equal(100);
done();
});
});

it('any alias', function(){
expect(async.any).to.equal(async.some);
});
Expand Down