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.
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.
There was a problem hiding this comment.
I doubt whether there is much need for onAbortResolution, onAbortStart, and abortResolver, so at this point I prefer to keep the API as simple and limited as possible.
I understand that it can be handy for unit testing, but to make the unit tests work I think it can be okish to use workarounds like using checking the stats after a setTimeout(..., 0) and things like that.
If we are to remove these callbacks then we should keep
abortResolverso the user knows when the abort operation concludes.
I may be misunderstanding, but I would think that moment is when the promise of the task at hand rejects with a timeout or cancel error? In my head, the abort functionality only has an API at the worker side and not at the pool side. The pool side can cancel or timeout a task. Then, internally, we either terminate the worker or abort the task (keeping the worker runnig). End result from the pool side is that the promise resolves with a cancel or timeout error.
There was a problem hiding this comment.
I doubt whether there is much need for onAbortResolution, onAbortStart, and abortResolver, so at this point I prefer to keep the API as simple and limited as possible. I understand that it can be handy for unit testing, but to make the unit tests work I think it can be okish to use workarounds like using checking the stats after a setTimeout(..., 0) and things like that.
We don't need them, as you said we can still test without them with setTimeout. I thought lifecycle hooks would be a useful addition to users but I see your point on not over complicating the api.
I may be misunderstanding, but I would think that moment is when the promise of the task at hand rejects with a timeout or cancel error? In my head, the abort functionality only has an API at the worker side and not at the pool side. The pool side can cancel or timeout a task. Then, internally, we either terminate the worker or abort the task (keeping the worker runnig). End result from the pool side is that the promise resolves with a cancel or timeout error.
This is mostly true, The main exception here is if the worker's abort operation times out and we have to reject the task promise from the pool side.
There was a problem hiding this comment.
Yes true, but the case of an abort operation timing out can be handled inside the WorkerHandler without need for a user facing API, right?
👍 ok lets remove the extra handlers again then.
This reverts commit 76cd6c6.
add back test updates
joshLong145
force-pushed
the
fix/abort-listener-execution
branch
from
9bd3e90 to
361bccd
Compare
ref: add more logs
…om worker from its timeout.
|
Hm I think I accidentally removed one of the inline comments, so if you mis something it was me, sorry 😅. Most important right now is to get on the same page regarding #484 (comment). |
Responded in thread, I can remove the |
PR for discussion on issue #479