cache icon indicating copy to clipboard operation
cache copied to clipboard

Exit after run to prevent hanging if there are active listeners

Open dhadka opened this issue 4 years ago • 7 comments

Closes https://github.com/actions/cache/issues/370

If there are active listeners after run() finishes, NodeJS will not exit. Generally, leaving an active listener is a bug that should be fixed, but to prevent this from causing the step to hang we can explicitly exit.

dhadka avatar Jul 14 '20 16:07 dhadka

Could you double-check the generated dist files? I wouldn't expect 100k lines to be changed for those.

joshmgross avatar Jul 14 '20 16:07 joshmgross

Whoops, this should be process.exit() so that we honor any process.exitCode previously set.

jclem avatar Jul 14 '20 17:07 jclem

Ah, as @ericsciple pointed out, we need to flush stdout/stderr, so just calling process.exit() isn't safe.

jclem avatar Jul 14 '20 17:07 jclem

Another option would be to set a timeout for writing stdout/stderr and then force exit:

const OUTPUT_TIMEOUT = 60_000

async function main() {
  // My action code
}

runSafeMain(main)

function runSafeMain(fn: (...args: any[]) => Promise<unknown>) {
  fn().finally(waitForOutput)
}

function waitForOutput() {
  const timeout = setTimeout(() => {
    process.exit()
  }, OUTPUT_TIMEOUT)

  // Ensure the timeout doesn't block the process from exiting normally
  timeout.unref()
}

See also this technique discussed in joyent/node-exuent.

jclem avatar Jul 14 '20 17:07 jclem

@joshmgross Fixed the dist/ files having tons of changes. Had it pointing to a local copy of @actions/toolkit. (The test that looks for uncommitted changes also detected this issue).

I'll wait until I hear back from @jclem and @ericsciple on how to handle flushing before doing anything with this.

dhadka avatar Jul 14 '20 17:07 dhadka

[warning]downloadCache failed: Request timeout: /758370072ebc48a1bae6dd969702511b/59eed5cca5c5ea119b0600155dafe08a?sv=2018-03-28&sr=b&sig=4O4ae0FEDWtyXHalSOBL7FMQNrPJdJSajOxwzi2WJsY%3D&se=2020-07-14T11%3A18%3A50Z&sp=r&rscl=x-e2eid-ca91503f-2fdf483a-86d18754-8bf30f0c
8
##[error]The operation was canceled.

It seems from the log the request got stuck after the timeout error. I suspect it's because of a bug in the http-client package we use (code here) but I'm not proficient enough in nodejs runtime to figure out where.. @jclem @joshmgross Any idea?

aiqiaoy avatar Jul 14 '20 18:07 aiqiaoy

@dhadka I'm changing default branch to main for cache so I rebased your PR to on top of main

aiqiaoy avatar Jul 15 '20 14:07 aiqiaoy

@vsvipul are you reactivating this PR?

bishal-pdMSFT avatar Nov 15 '22 03:11 bishal-pdMSFT

@bishal-pdMSFT I'm not sure if this is still required. I'll check and follow up accordingly.

vsvipul avatar Nov 15 '22 05:11 vsvipul

Closing this for now, as we aren't facing any issues related to this.

vsvipul avatar Jan 04 '23 10:01 vsvipul