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

Removing internal setImmediate's #704

Closed
wants to merge 5 commits into from

Conversation

@aearly
Copy link
Collaborator

aearly commented Jan 20, 2015

From the discussion in #696 , this PR removes most internal setImmediates to remove the unnecessary performance hit when you're using asynchronous functions. It leaves it to the user to guard against stack overflows with async.setImmediate or async.nextTick if they need to callback synchronously. This is a breaking change since there are many people relying on automatic deferral in waterfall and auto.

I had to refactor the logic in the functions that used setImmediate because the internal logic was relying on those deferrals. The timing of certain callbacks and events has also changed, so there will be further subtle breaking changes there. If the functions you pass to async were actually asynchronous, things should work as they always have.

I left the deferrals in for priorityQueue and cargo, because those seemed to be needed for them to actually work. They need to defer in order to become saturated so the priorities and payload sizes can take effect.

I also see that queue, priorityQueue, and cargo could likely be refactored to use the same underlying logic, albeit a bit more complicated. queue would have a payload size as well as a concurrency, and q.push would have an optional priority parameter.


```js
async.eachSeries(hugeArray, function iterator(item, callback) {
if (item.somePredicate()) {

This comment has been minimized.

Copy link
@charlierudolph

charlierudolph Jun 2, 2015

Contributor

In order to match the example above, I think this should be changed to:

if (matchesSomething(item)) {
```js
async.eachSeries(hugeArray, function iterator(item, callback) {
if (item.somePredicate()) {
async,nextTick(function () {

This comment has been minimized.

Copy link
@charlierudolph

charlierudolph Jun 2, 2015

Contributor

. instead of a ,

This comment has been minimized.

Copy link
@aearly

aearly Jun 2, 2015

Author Collaborator

These were fixed in the version that made it to master: b96705a

@@ -501,6 +507,7 @@ exports['auto error should pass partial results'] = function(test) {
}]
},
function(err, results){
console.log("auto error done")

This comment has been minimized.

Copy link
@charlierudolph

charlierudolph Jun 2, 2015

Contributor

this should probably be removed

@aearly aearly added this to the 2.0 milestone Jul 2, 2015
@aearly

This comment has been minimized.

Copy link
Collaborator Author

aearly commented Oct 25, 2015

Closing this for now. The doc updates were already added. Things have diverged so much this PR isn't useful -- we'll track this with #696 for v2.0.

@aearly aearly closed this Oct 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.