Skip to content
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

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

AriPerkkio
Copy link
Member

@AriPerkkio AriPerkkio commented Mar 4, 2023

Example usage:

try {
  await pool.run(data, { transferList: [workerPort], name })
}
catch (error) {
  if(/Failed to terminate worker/.test(error.message)) {
    // Do something with this information, e.g. more detailed error message:
    console.log(`Failed to terminate worker when running ${data.filename}! Something in that file prevents worker from terminating.`)
  }
}


const timer = timeout
? setTimeout(
() => reject(new Error('Failed to terminate worker')),
Copy link
Member Author

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's worker.terminate was taking too long or got stuck.

setTimeout(() => {
  this.worker.postMessage('TERMINATE_NOW')
  reject(new Error('Failed to terminate worker')
})

Worker would catch this:

// worker.ts
parentPort.on('message', (message) => {
  // Main thread tried to terminate this worker but failed, let's force the exit here in worker
  if (message === 'TERMINATE_NOW') {
    process.exit()
  }
})

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.

@AriPerkkio AriPerkkio marked this pull request as ready for review March 5, 2023 07:05
)
: null

this.worker.terminate().then(() => {
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot await as the outer Promise has to be the return value.

This outer Promise is required since setTimeout is used to decide when destroy function should reject. This would not work:

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 runTask:

tinypool/src/index.ts

Lines 832 to 838 in d5e5738

let resolve: (result: any) => void
let reject: (err: Error) => void
// eslint-disable-next-line
const ret = new Promise((res, rej) => {
resolve = res
reject = rej
})

@Aslemammad
Copy link
Member

Thank you so much @AriPerkkio, let's merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: terminateTimeout options that decides when slow/stuck worker termination should be error
2 participants