eachSeries should finish if underlying array is modified

[abenhamdine]

What version of async are you using?
2.2.0

Which environment did the issue occur in (Node version/browser version)
nodejs 7.7.3

What did you do? Please include a minimal reproducable case illustrating issue.

	it("async.eachSeries : add an element in underlying array while iterating should finish iteration, per docs", function(testDone) {

		var data = [1, 2, 3, 4, 5];
		var results = [];
		var iterator = function(collection, item, eachDone) {

			console.log("we process item =" + item);

			results.push(item);
			if (item === 2) {
				// we add "new" after 2
				collection.splice(2, 0, "new");
			}
			return eachDone();

		};

		async.eachSeries(data, iterator.bind(null, data), function(err) {

			expect(err).to.be.not.exist;

			// we expect the iteration to finish after have processed value 2, but actually, the iteration continues, but with respect to the original array length
			expect(results).to.have.length(2);

			testDone();
		});

	});

What did you expect to happen?

According to changelog v1.10 : eachSeries and family will finish if the underlying array is modified during execution (#557)
I did not find more recent reference to this topic.

So I expected the iteration to stop after have modified the array

What was the actual result?

The iterations continue, but only according to the original length of the array

FWIW, I need to add elements (and so iterations) to the array while iterating, that's why I was testing this behaviour.

[megawac]

This behaviour was removed to align with array#forEach which as a safety feature will iterate only the indexes in the array when the forEach function was called.

I would consider using queue instead of eachSeries which supports use cases like this.

[abenhamdine]

Ah yes queue seems a good candidate for my use case, thx 👍 .
Though shouldn't the docs be updated to warn about the immutability of the collection ? I dont' find any reference to this, eg in http://caolan.github.io/async/docs.html#each, except the changelog of old v1.10 I mentioned.

If you are open to a PR for the docs, where in the docs do you think it would be the most relevant ?

[aearly]

Good question. It would be really tedious to note that detail in each of the collections methods. How about in the Collections section header, defined in index.js? http://caolan.github.io/async/docs.html#collections

[abenhamdine]

Yes, the Collections section headers looks like a good place for it, though I think many users, as I do, only use the search field, and are not aware of the section header (to be frank, I never thought about clicking in the Collections word).

[nakiabrewer]

Hi,

I've been working with Async today and have come across this as well.
Unless i missed it (likely) this should really be stated on the documentation.

I am struggling to understand how I could use queue to try and achieve this? (beginner, be kind)

               var pages = ['1']; // Start with page 1, additional pages are added to this array per iteration
		var records = [];
		// Now loop through each of the pages
		async.eachSeries(pages, function(value, callback){
			// Configure the request options
				console.log(value);
				var options = {
				  url: 'https://forms.logiforms.com/api/1.0/form/' + formId + '/data?page=' + value,
				  method: 'GET',
				  json: true,
				  proxy: proxyAuthenticationString,
				  headers: {
				    'Authorization': apiKey
		 			}
				};
				// Now complete the request
				request(options, function(err, res, body) {
					// Something went wrong  
				    if (err) {
				    	logger.info(err);
				    	callback('ERROR', null);
				    };
				    // All went as planned, push into objects
			    	pages.push(body.data.next_page);
			    	logger.info(pages);
			    	logger.info(body.data.next_page);
					callback(null, body);
				}); 
		}, function(err){

			}
		)

[aearly]

@nakiabrewer doWhilst or queue might be better suited to your task. (Iterate while body.data.next_page exists, or keep pushing new pages to the queue.)