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

queue.remove() #1397

Merged
merged 3 commits into from
Apr 9, 2017
Merged

queue.remove() #1397

merged 3 commits into from
Apr 9, 2017

Conversation

aearly
Copy link
Collaborator

@aearly aearly commented Apr 7, 2017

Closes #1391 .

Adds a remove function to queues. Without it, there is no way to arbitrarily remove items from the queue. In Async 1.x and below, the tasks list was just an array, so you could splice as you needed. This adds a filter function to the DoublyLinkedList class that is used by q.remove.

I also added some unit tests for DLL itself. I also noticed that the prior implementation of empty could cause a memory leak. I think the lingering chain of prev/next references could fool GC and keep the objects around.

@aearly aearly changed the title Linked list methods queue.remove() Apr 7, 2017
return arr;
}

DLL.prototype.filter = function (testFn) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this destructive? If so I'd rather not call it filter, but remove

Copy link
Collaborator Author

@aearly aearly Apr 9, 2017

Choose a reason for hiding this comment

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

I wanted to call it filter so that if someone wants to swap out the implementation of _tasks with an array, they could. e.g.:

var q = async.queue(worker);
q._tasks = [];

I remember we got an issue a while back after 2.0 because some people were doing custom splicing in the queue, which they couldn't do with the linked list.

It is strange that it doesn't return a new list (destructive) even though it's called filter, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually after more thought, remove is a better name. If someone is using an array implementation, they're likely doing their own array splicing somehow anyway.

remove: function (testFn) {
q._tasks = q._tasks.filter(function (node) {
return !testFn(node);
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: missing semi

@aearly aearly merged commit 8f58dbd into master Apr 9, 2017
@aearly aearly deleted the linked-list-methods branch April 9, 2017 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants