autostart option - Githubissues

jessetane / queue

Asynchronous function queue with adjustable concurrency

MIT License

764 stars 66 forks source link

autostart option #28

Closed tnguyen14 closed 7 years ago

tnguyen14 commented 7 years ago

my attempt at having an autostart option.

not yet familiar with the code base, so please pardon any oversight.

if the implementation is acceptable, I'll make additional changes to the docs.

/cc @jessetane

coveralls commented 7 years ago

Coverage decreased (-1.1%) to 98.864% when pulling c383c8a37767287fe8c38b06926c778ec968a51d on tnguyen14:master into 9d9a68eb9ab7a0bd3497bd3bd132173ccff94624 on jessetane:master.

jessetane commented 7 years ago

Hi @tnguyen14, thanks for the PR - super busy next couple of days but will take a closer look and get back to you as soon as I can. If anyone else has thoughts or wants to help review that would be welcome.

jessetane commented 7 years ago

I've had a look and have a few thoughts.

First, this is how queue used to work - check out c6c3ae49f28447e19cc135685f63d978ff507dda. The reason I changed it is because in real life, third party libraries you might use queue with can and will occasionally invoke their callbacks synchronously. This results in spurious and difficult to track down bugs that can generally be avoided if the user is forced to call start by hand. Footgun aside, I do appreciate the use case here so I'm ok with adding it behind an option.

Second, I agree that the end event issue you pointed out is weird, especially for the autostart use case where having multiple end events may be useful. Would you mind changing this behavior as well? We will need to cut a new major release since this would break the API but given the potential dangers that come along with the autostart feature I think not an inappropriate time to do so.

jessetane commented 7 years ago

RE end event: was I reading your test incorrectly and in fact you are just experiencing the sync callback issue? Hm... let me know what you find.

coveralls commented 7 years ago

Coverage decreased (-1.1%) to 98.876% when pulling 165f92b09a5fabd7fc930c35ca239ab3f3254b84 on tnguyen14:master into 9d9a68eb9ab7a0bd3497bd3bd132173ccff94624 on jessetane:master.

tnguyen14 commented 7 years ago

@jessetane I've made the following changes:

return the result of the array method, instead of the start method, as you called out.
have the end event listener be registered first. this was the reason it was only called once before, because it has not been registered by the time the first 2 jobs were executed. now, it will be called 3 times, because start is now called for each time the jobs array is changed.

I'm not 100% familiar with the project - could you explain a bit more what you mean by calling callback synchronously, and how that was causing the bugs?

jessetane commented 7 years ago

Great work, thanks again and sorry for the delay. By sync (vs async) I mean whether a job's callback is called before (or after) the job itself returns. As you noticed this is what caused the strange end event behavior, not an actual problem with the library.

jessetane commented 7 years ago

Going to do a little a cleanup and try to get test coverage back up to 100% but will publish shortly.

jessetane commented 7 years ago

Published as 4.1.0