onEmpty/onIdle misusing Promises

[aikar]

trafficstars

These methods are event oriented and should not have been using promises.

A promise is intended to only be resolved once, however this library is expecting to resolve them multiple times. https://stackoverflow.com/questions/20328073/is-it-safe-to-resolve-a-promise-multiple-times

these API's need to be altered to queue.onIdle(() => { }); and register an array of callbacks, or ideally, use an EventEmitter style API and do queue.on('idle', () => { });

these existing API's can be altered to forward to the new preferred API by returning something like return { then(cb) { this.on('idle', cb); return this; } catch (e) { } //unused, but part of promise expectation }

[sindresorhus]

A promise is intended to only be resolved once, however this library is expecting to resolve them multiple times.

A promise is not intended to resolve once, it can only resolve once. That's irrelevant though as we're not requiring you to resolve a promise multiple times. We're returning a fresh promise each time you call the method. This is a common pattern.

[aikar]

This relates to https://github.com/sindresorhus/p-queue/commit/e91d91b221c66f1068589a926a86cfe54f14256c

That commit resolves the double resolve issue.

However, if you rather keep the promise style API, i would suggest making it clearer in the documentation that if you want to always know about idle, you need to re-register the onIdle/onEmpty handler again, that calling this only informs you of the next idle.

I know this is expected behavior to anyone who understands the details of promises, but there will be plenty of users of this API who do not know that, or people who in my case, thought maybe you were doing something really hacky to call the .then() supplied callback multiple times.

The documentation was not clear to me that the intent of the API was a one time fire.

But an event emitter style with queue.once("idle", () =>{}); would satisfy this same desire ("I only need to know about the next idle"), and queue.on("idle"); satisfies "I need to know about every idle"

So I still recommend migrating the API to use an emitter, and make .onIdle()/.onEmpty() be forwarders to .once()

[sindresorhus]

I went with promises initially so you can await them easily and I didn't see think of any use-case for needing an event emitter. And yes, we can document it better.

---

So I still recommend migrating the API to use an emitter

What is your use-case for this? I'm just curious.

[aikar]

I'm currently running 2 priority queues, with different concurrency levels, one high one low.

Because browsers have a limit on how many requests, can be pending to a single domain, I put "really low priority" items into the low priority queue with a lower concurrency level, ensuring there is always open room for a high priority request to come in and start.

I'm trying to avoid the case where there are many slow requests running, using up all available connections, and a higher priority request comes in, but is blocked by the slow ones.

Anytime the high queue takes work, I pause the low queue (letting the overlap in concurrency shift from low to high in the event the high queue gets a lot of requests), and once the high queue goes idle, I resume the low queue.

using a single queue works fine for controlling priority, but nothing would stop 6 low priority tasks that take say 5s+ to execute, from holding the queue up, making the high priority task still have to wait.

So the double queue approach solved this issue for me.

Another case would be even for non request oriented tasks, say you have some background task/poller/monitor that should run anytime the queue is idle, and paused anytime the queue has work, having easy start/stop events makes it easy to react to the queue changing state and control the external operation.

For example, a web based game may want to pause some CPU heavy task while high priority work is occuring.

but for onIdle/onEmpty, I understand that intent and that's fine now that the above linked commit fixed the implementation bug, just would be good to document the intent of how that API is to be used, and suggesting an event emitter style for people who just want to know event style state changes.

[maxfi]

I also thought the same as @aikar on my first skim of the docs that queue.onIdle would register a callback.

EventEmitter style API and do queue.on('idle', () => { });

This would be awesome! An example use case might be:

someEventEmitter.on('new-doc', (doc) => 
  queue.add(
    () => db.add(doc)
  )
) 

queue.on('start', log('starting'))
queue.on('added', log('added'))
queue.on('resolved', log('resolved'))
queue.on('empty', log('empty'))
queue.on('idle', log('idle'))
// => 
// idle

someEventEmitter.emit('new-doc', ({id: 1}))
// =>
// starting
// added {id: 1}
// empty
// resolved 'database-id-1'
// idle

[philipp-winterle]

Had also an issue with the onIdle and onEmpty calls. See my Testcase: https://repl.it/@RandomPxl/p-queue-Test

I would expect onIdle to be called every time my queue runs idle to know when things are done. If I use start even with items added I immediatly get onIdle called and then never again. So I would add onIdle multiple times which would cause it to be called multiple times.

I dont know your use cases but I would not like to add onIdle/Empty more than once.

Iam also not sure about it but wouldnt it be more like: onIdle == empty && idle instead of onIdle == idle

Reason is a queue is never idling when there are still items left in the queue. Even when they are not processing

[szmarczak]

I propose queue.isIdle and queue.onIdle([callback])

[szmarczak]

@maxfi It's not safe.

queue.on('idle', () => {
	queue.add(item);
	// it's not idle anymore
});

[jordanbtucker]

I disagree with the other sentiments here. It was obvious to me that onIdle and onEmpty return single use promises because I understand how promises work.

If you call onIdle, it will return a promise that will resolve when the queue is done. If you add more items to the queue after the promise resolves, then you need to get a new promise by calling onIdle again. It's really quite simple.

I can also see how making PQueue an EventEmitter with an on('idle') event could be useful too, however.