setup-node icon indicating copy to clipboard operation
setup-node copied to clipboard

Explicitly exit the process to not wait for hanging promises

Open xiniria opened this issue 2 years ago • 11 comments

Description: Since Node.js 19, the default for keepAlive was changed from false to true. As explained in this similar issue for the Ruby setup action, this causes the Post setup Node action to hang for somewhere between 1 and 5 minutes after running. This PR explicitly exits the process once the run function is completed, to not wait for hanging promises.

Related issue: Fixes #878

Check list:

  • [ ] Mark if documentation changes are required.
  • [x] Mark if tests were added or updated to cover the changes.

xiniria avatar Nov 27 '23 15:11 xiniria

Change works as a quick fix, but it's worth noting the proper fix would be to merge https://github.com/actions/toolkit/pull/1572 (or a similar fix on actions/toolkit level) otherwise we'd be copy pasting this exit process hack to tons of other actions/* that cache internally as they update to Node 20.

Bo98 avatar Nov 29 '23 01:11 Bo98

Change works as a quick fix, but it's worth noting the proper fix would be to merge actions/toolkit#1572 (or a similar fix on actions/toolkit level) otherwise we'd be copy pasting this exit process hack to tons of other actions/* that cache internally as they update to Node 20.

I totally agree, but since we have no leverage over when (or even if) this other PR is merged, this should improve things for a lot of users out there in the meantine, don't you think?

xiniria avatar Nov 29 '23 09:11 xiniria

Yeah for sure. Though given GitHub control both repositories, there's a chance the right person will see it here.

External contributions to actions/toolkit are not currently being reviewed (and haven't for a year) so it felt it was worth mentioning in case they want to make a similar fix themselves. If the toolkit and setup-node teams are totally separate and usually don't collaborate then yeah I imagine merging your workaround makes sense.

Bo98 avatar Nov 29 '23 11:11 Bo98

@nikolai-laevskii could you have a look at the PR?

xiniria avatar Dec 04 '23 12:12 xiniria

@nikolai-laevskii any news on this PR that could help us a lot on our delivery?

xaviermarchal avatar Dec 09 '23 13:12 xaviermarchal

It would great to have this simple fix merged. It's not blocking, but somehow nice to have, as these ~5 Minutes do sum up to a huge number of minutes for teams with large number of developers and multiple builds with every commit. Thank you @xiniria the PR.

ehasnain avatar May 15 '24 12:05 ehasnain

It's already fixed: https://github.com/actions/setup-node/commit/ec97f37504b0cca1fbc763cc0575585d10020c22

And now double-fixed by the @actions/http-client update: https://github.com/actions/setup-node/commit/c2ac33f2c62f978d6c944d9648125a294e56dc0b

Bo98 avatar May 15 '24 12:05 Bo98

Hi @Bo98 thanks for the quick update. Not sure why but we are still experiencing 4-5 Minutes of duration in the post step.

image image image

We are using actions/setup-node@v4 with node-version: 20.11 on multiple pipelines. Maybe here is not the right spot for the bug/issue reporting, but I landed here on going through the past issues and PRs related to the same.

ehasnain avatar May 15 '24 13:05 ehasnain

1GB cache is fairly large. Can you confirm with the "Show timestamps" option that it's actually hanging after the Cached saved with key line?

Bo98 avatar May 15 '24 13:05 Bo98

Hi @Bo98, your guess is right. The size of 1 GB is actually taking quite long. The step ends right after Cache saved with key: ... is logged. image

Although a compression of similar data (size and number of files), run manually on the same server and disk takes just seconds. I'll try to dig in further. Thank you already for looking into this.

ehasnain avatar May 16 '24 07:05 ehasnain