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

Fix a bug that forced the init Action to run for at least two minutes on JavaScript #1494

Merged
merged 4 commits into from
Jan 20, 2023

Conversation

henrymercer
Copy link
Contributor

The bug is that we wait for timeouts to elapse at the end of the init Action, even if the operation we were applying the timeout to already finished.

Context

Some of the Actions caching APIs we call don't have reliable timeouts, so we implement our own timeout on top of this. Once this timeout elapses, the Action continues executing without downloading / uploading the cache. However, once the top level of the Action has finished executing, the Node runtime will continue waiting on the background tasks to finish executing. We get around this by adding a call to util.checkForTimeout() to the end of the top level of the Action. This kills the Node process after 30 seconds if there was a timeout at some point during the Action's execution (see #1354 for more).

However it turns out we're liable to the same waiting problem with the timeouts themselves. If the cache operation finishes before the timeout, we'll still wait for the timeout itself to finish executing before the Action exits. This impacts JavaScript runs, which use TRAP caching. The TRAP caching timeout is currently two minutes, so the init Action will now wait for up to 2 minutes to finish on any workflows that run a language that supports TRAP caching.

Fix

To fix this, we immediately unref the timer so that Node won't wait for it to finish.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@henrymercer henrymercer requested a review from a team as a code owner January 19, 2023 20:07
src/util.ts Outdated Show resolved Hide resolved
adityasharad
adityasharad previously approved these changes Jan 19, 2023
aeisenberg
aeisenberg previously approved these changes Jan 19, 2023
@henrymercer henrymercer merged commit 6456115 into main Jan 20, 2023
@henrymercer henrymercer deleted the henrymercer/avoid-waiting-for-timeout branch January 20, 2023 17:24
@github-actions github-actions bot mentioned this pull request Jan 26, 2023
6 tasks
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.

3 participants