-
-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: terminateTimeout
option
#50
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -122,6 +122,7 @@ interface Options { | |||||||||||||||
minThreads?: number | ||||||||||||||||
maxThreads?: number | ||||||||||||||||
idleTimeout?: number | ||||||||||||||||
terminateTimeout?: number | ||||||||||||||||
maxQueue?: number | 'auto' | ||||||||||||||||
concurrentTasksPerWorker?: number | ||||||||||||||||
useAtomics?: boolean | ||||||||||||||||
|
@@ -447,14 +448,38 @@ class WorkerInfo extends AsynchronouslyCreatedResource { | |||||||||||||||
) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
async destroy(): Promise<void> { | ||||||||||||||||
await this.worker.terminate() | ||||||||||||||||
this.port.close() | ||||||||||||||||
this.clearIdleTimeout() | ||||||||||||||||
for (const taskInfo of this.taskInfos.values()) { | ||||||||||||||||
taskInfo.done(Errors.ThreadTermination()) | ||||||||||||||||
} | ||||||||||||||||
this.taskInfos.clear() | ||||||||||||||||
async destroy(timeout?: number): Promise<void> { | ||||||||||||||||
let resolve: () => void | ||||||||||||||||
let reject: (err: Error) => void | ||||||||||||||||
|
||||||||||||||||
const ret = new Promise<void>((res, rej) => { | ||||||||||||||||
resolve = res | ||||||||||||||||
reject = rej | ||||||||||||||||
}) | ||||||||||||||||
|
||||||||||||||||
const timer = timeout | ||||||||||||||||
? setTimeout( | ||||||||||||||||
() => reject(new Error('Failed to terminate worker')), | ||||||||||||||||
timeout | ||||||||||||||||
) | ||||||||||||||||
: null | ||||||||||||||||
|
||||||||||||||||
this.worker.terminate().then(() => { | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't we just await the terminate instead of using the then keyword? if not, why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We cannot This outer Promise is required since async function destroy() {
setTimeout(() => {
throw new Error("Timeout error")
}, 1000);
await sleep(2000);
} destroy().then(() => console.log("success")).catch(() => console.log("Failed"))
> Uncaught Error: Timeout error
> success Wrapping everything in a outer Promise helps: async function destroy() {
let resolve, reject;
const outerPromise = new Promise((res, rej) => {
resolve = res
reject = rej
})
setTimeout(() => {
reject(new Error("Timeout error"))
}, 1000);
sleep(2000).then(resolve);
return outerPromise;
} destroy().then(() => console.log("success")).catch(() => console.log("Failed"))
> Failed Similar pattern is used in Lines 832 to 838 in d5e5738
|
||||||||||||||||
if (timer !== null) { | ||||||||||||||||
clearTimeout(timer) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
this.port.close() | ||||||||||||||||
this.clearIdleTimeout() | ||||||||||||||||
for (const taskInfo of this.taskInfos.values()) { | ||||||||||||||||
taskInfo.done(Errors.ThreadTermination()) | ||||||||||||||||
} | ||||||||||||||||
this.taskInfos.clear() | ||||||||||||||||
|
||||||||||||||||
resolve() | ||||||||||||||||
}) | ||||||||||||||||
|
||||||||||||||||
return ret | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
clearIdleTimeout(): void { | ||||||||||||||||
|
@@ -771,12 +796,12 @@ class ThreadPool { | |||||||||||||||
} | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
_removeWorker(workerInfo: WorkerInfo): void { | ||||||||||||||||
_removeWorker(workerInfo: WorkerInfo): Promise<void> { | ||||||||||||||||
workerInfo.freeWorkerId() | ||||||||||||||||
|
||||||||||||||||
workerInfo.destroy() | ||||||||||||||||
|
||||||||||||||||
this.workers.delete(workerInfo) | ||||||||||||||||
|
||||||||||||||||
return workerInfo.destroy(this.options.terminateTimeout) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
_onWorkerAvailable(workerInfo: WorkerInfo): void { | ||||||||||||||||
|
@@ -845,14 +870,16 @@ class ThreadPool { | |||||||||||||||
this.completed++ | ||||||||||||||||
if (err !== null) { | ||||||||||||||||
reject(err) | ||||||||||||||||
} else { | ||||||||||||||||
resolve(result) | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
// When `isolateWorkers` is enabled, remove the worker after task is finished | ||||||||||||||||
if (this.options.isolateWorkers && taskInfo.workerInfo) { | ||||||||||||||||
this._removeWorker(taskInfo.workerInfo) | ||||||||||||||||
this._ensureEnoughWorkersForTaskQueue() | ||||||||||||||||
.then(() => this._ensureEnoughWorkersForTaskQueue()) | ||||||||||||||||
.then(() => resolve(result)) | ||||||||||||||||
.catch(reject) | ||||||||||||||||
} else { | ||||||||||||||||
resolve(result) | ||||||||||||||||
} | ||||||||||||||||
}, | ||||||||||||||||
signal, | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import { dirname, resolve } from 'path' | ||
import { Tinypool } from 'tinypool' | ||
import { fileURLToPath } from 'url' | ||
|
||
const __dirname = dirname(fileURLToPath(import.meta.url)) | ||
const cleanups: (() => Promise<unknown>)[] = [] | ||
|
||
afterEach(async () => { | ||
await Promise.all(cleanups.splice(0).map((cleanup) => cleanup())) | ||
}) | ||
|
||
test('termination timeout throws when worker does not terminate in time', async () => { | ||
const pool = new Tinypool({ | ||
filename: resolve(__dirname, 'fixtures/sleep.js'), | ||
terminateTimeout: 10, | ||
minThreads: 1, | ||
maxThreads: 2, | ||
isolateWorkers: true, | ||
}) | ||
|
||
expect(pool.threads.length).toBe(1) | ||
|
||
const worker = pool.threads[0] | ||
expect(worker).toBeTruthy() | ||
|
||
cleanups.push(worker!.terminate.bind(worker)) | ||
worker!.terminate = () => new Promise(() => {}) | ||
|
||
await expect(pool.run('default')).rejects.toThrowError( | ||
'Failed to terminate worker' | ||
) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea I had was that here we could tell the worker to
process.exit
if main thread'sworker.terminate
was taking too long or got stuck.Worker would catch this:
But it seems that this does not work. The worker never receives the message. I guess the
worker.terminate()
has already closed the message channel even though it has not yet resolved completely.