Fix/abort listener execution #484
Conversation
…workerpool into fix/abort-listener-execution
…rt-listener-execution
update types remove logging
update execOptions update abort tests
| * @property {PromiseLike<void>} [abortPromise] PromiseLike Object which resolves or rejects when the abort operation concludes. | ||
| * | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {Object} AbortResolutionArgs | ||
| * @property {Error | undefined} [error] An error which occured during the abort operation. If an error did not occure the value will be `undefined`. | ||
| * @property {number} [id] identifier of the task. | ||
| * @property {boolean} [isTerminating] A flag which indicates the termination status of the worker which ececuted the task. | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {Object} ExecOptions | ||
| * @property {(payload: any) => unknown} [on] An event listener, to handle events sent by the worker for this execution. | ||
| * @property {Object[]} [transfer] A list of transferable objects to send to the worker. Not supported by `process` worker type. See ./examples/transferableObjects.js for usage. | ||
| * @property {(payload: any) => AbortResolutionArgs} [onAbortResolution] An event listener triggered when whenever an abort operation concludes. | ||
| * @property {(payload: AbortStartArgs) => void} [onAbortStart] An event listener triggered when a task throws a Timeout or Canceltion exception and cleanup is starting. | ||
| * @property {{ promise: import('./Promise.js').Promise<any, Error>; resolve: Function; reject: Function; }} [abortResolver] Defered Promise which resolves or rejects when the abort operation for the task concludes. |
There was a problem hiding this comment.
I am torn on if this is a good mechanism for allowing callers access to the promise which controls the abort operation. In AbortResolutionArgs we provide the promise for tracking the abort operations. We are able to provide the promise from the tracking deferred promise created internally.
We also allow the deferred promise for the abort operation to be provided through the ExecOptions. Since we need a deferred promise we cannot simply provide an instance of Promise.
This means the promise for the abort operation is exposed as two different types. Which is not ideal, but I am unsure if there is a way to unify them without breaking backwards compatibility.
There was a problem hiding this comment.
I'm not sure how abortResolver would work, isn't it duplicate with onAbortResolution?
There was a problem hiding this comment.
onAbortResolution is called after the abort handlers have executed and passes the taskId and error if the abort handlers threw an error, and a flag to determine if the worker is shutting down or not.
abortResolver is just allowing the user to provide the promise that is used as the tracking promise for a tasks abort operation.
The idea is to give control of the promise to the caller, so they do not need to get it from event args. but it can be misused easily.
| settlePromise, | ||
| timeoutPromise | ||
| ]).then(function() { | ||
| return new Promise(function (resolve, reject) { |
There was a problem hiding this comment.
Not sure, but can we use Promise.any([...]) here?
There was a problem hiding this comment.
We can, but I was trying to not use any implementation of Promise from outside of the repo.
| /** | ||
| * @typedef {Object} ExecOptions | ||
| * @property {(payload: any) => unknown} [on] An event listener, to handle events sent by the worker for this execution. | ||
| * @property {Object[]} [transfer] A list of transferable objects to send to the worker. Not supported by `process` worker type. See ./examples/transferableObjects.js for usage. | ||
| * @property {(payload: any) => AbortResolutionArgs} [onAbortResolution] An event listener triggered when whenever an abort operation concludes. |
There was a problem hiding this comment.
Is there an actual use case for the onAbortResolution and onAbortStart callbacks? If there is no clear use case I would prefer to not implement it right now, to keep things simple.
There was a problem hiding this comment.
The main usage is to give more context to the user on life cycle of the task. but onAbortStart does provide access the to promise.
It is also useful in tests to confirm that after the resolution of the abort operation the busyWorker count is correct as this cannot be asserted on from within the resolution of cancel or timeout promises.
If we are to remove these callbacks then we should keep abortResolver so the user knows when the abort operation concludes.
| * @property {PromiseLike<void>} [abortPromise] PromiseLike Object which resolves or rejects when the abort operation concludes. | ||
| * | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {Object} AbortResolutionArgs | ||
| * @property {Error | undefined} [error] An error which occured during the abort operation. If an error did not occure the value will be `undefined`. | ||
| * @property {number} [id] identifier of the task. | ||
| * @property {boolean} [isTerminating] A flag which indicates the termination status of the worker which ececuted the task. | ||
| */ | ||
|
|
||
| /** | ||
| * @typedef {Object} ExecOptions | ||
| * @property {(payload: any) => unknown} [on] An event listener, to handle events sent by the worker for this execution. | ||
| * @property {Object[]} [transfer] A list of transferable objects to send to the worker. Not supported by `process` worker type. See ./examples/transferableObjects.js for usage. | ||
| * @property {(payload: any) => AbortResolutionArgs} [onAbortResolution] An event listener triggered when whenever an abort operation concludes. | ||
| * @property {(payload: AbortStartArgs) => void} [onAbortStart] An event listener triggered when a task throws a Timeout or Canceltion exception and cleanup is starting. | ||
| * @property {{ promise: import('./Promise.js').Promise<any, Error>; resolve: Function; reject: Function; }} [abortResolver] Defered Promise which resolves or rejects when the abort operation for the task concludes. |
There was a problem hiding this comment.
I'm not sure how abortResolver would work, isn't it duplicate with onAbortResolution?
test/Pool.test.js
Outdated
| var {Promise} = require('../src/Promise'); | ||
| var Pool = require('../src/Pool'); | ||
| var tryRequire = require('./utils').tryRequire | ||
| var assert = require("assert"); |
There was a problem hiding this comment.
I see you have different coding style configured in your IDE 😅 . We should really setup a linter for the project, let me see if I can set up Biomejs (if so we can run it after merging this PR, otherwise we'll have a lot of merge conflicts).
Is it possible to revert the coding style changes, so I can see which unit tests are added/updated?
There was a problem hiding this comment.
Yes sorry I switched Editors recently and format on save was default enabled...
There was a problem hiding this comment.
I reverted back to the upstream master and applied my changes to the abort handler test suite. there are a couple formatting changes that I'll clean up. but the tests where largely re written to wait until the abort operation concludes to assert on the pool status. Which makes it a larger diff.
This reverts commit 76cd6c6.
add back test updates
joshLong145
force-pushed
the
fix/abort-listener-execution
branch
from
9bd3e90 to
361bccd
Compare
PR for discussion on issue #479