Possible bugs in the code.

[iSuslov]

While I'm rewriting the code to Typescript, I found two things that I would rather ask here since it looks like bugs.

1. Lets take a look here:

workerpool/src/WorkerHandler.js

Lines 483 to 494 in 8219f7d

	* @param {number} [timeout] If provided and non-zero, worker termination promise will be rejected
	* after timeout if worker process has not been terminated.
	* @return {Promise.<WorkerHandler, Error>}
	*/
	WorkerHandler.prototype.terminateAndNotify = function (force, timeout) {
	var resolver = Promise.defer();
	if (timeout) {
	resolver.promise.timeout = timeout;
	}
	this.terminate(force, function(err, worker) {
	if (err) {
	resolver.reject(err);

On the line 490 there is a assignment resolver.promise.timeout = timeout;. Doc of the method says timeout param is a number, but Promise object has a method called timeout. Seems like an attempt to override a method with a number. Task object however has a property timeout:number. Luckily it seems nowhere in the code this method WorkerHandler.prototype.terminateAndNotify gets called with the second argument.

Is it a bug in abandoned code?

2. Second one. Lets take a look at how TERMINATE_METHOD_ID is used.

workerpool/src/WorkerHandler.js

Line 11 in 8219f7d

var TERMINATE_METHOD_ID = '__workerpool-terminate__';

workerpool/src/WorkerHandler.js

Lines 450 to 454 in 8219f7d

	if (this.worker.ready) {
	this.worker.send(TERMINATE_METHOD_ID);
	} else {
	this.requestQueue.push(TERMINATE_METHOD_ID);
	}

There is no problem when it gets directly sent to a worker, however when it gets added to the requestQueue there is a problem, because this is how requestQueue is being processed:

workerpool/src/WorkerHandler.js

Lines 272 to 278 in 8219f7d

	// send all queued requests to worker
	function dispatchQueuedRequests()
	{
	for(const request of me.requestQueue.splice(0)) {
	me.worker.send(request.message, request.transfer);
	}
	}

Looks like instead of TERMINATE_METHOD_ID undefined gets sent. Correct me if I'm wrong please.

[josdejong]

Good finds Ivan!

1. I think this is indeed a bug. The method terminateAndNotify is called with the second timeout argument only from the public Pool.terminate method. I commented the line resolver.promise.timeout = timeout;, but still all tests pass. I expect that the terminate timeout simply doesn't work and isn't tested. The solution most likely is to change that line to resolver.promise.timeout(timeout).

2. I think you're right, it should be something like this.requestQueue.push({ message: TERMINATE_METHOD_ID });.

I think it's best to address these bugs separately from the refactor and make sure they are properly tested.

[Michsior14]

Both bugs have been resolved :)

[josdejong]

Both issues are fixed now in v6.4.2