-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Explicitly exit the process to not wait for hanging promises #907
base: main
Are you sure you want to change the base?
Conversation
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 |
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? |
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. |
@nikolai-laevskii could you have a look at the PR? |
@nikolai-laevskii any news on this PR that could help us a lot on our delivery? |
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. |
Hi @Bo98 thanks for the quick update. Not sure why but we are still experiencing 4-5 Minutes of duration in the post step. We are using actions/setup-node@v4 with node-version: 20.11 on multiple pipelines. |
1GB cache is fairly large. Can you confirm with the "Show timestamps" option that it's actually hanging after the |
Hi @Bo98, your guess is right. The size of 1 GB is actually taking quite long. The step ends right after 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. |
Description:
Since Node.js 19, the default for
keepAlive
was changed fromfalse
totrue
. 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 therun
function is completed, to not wait for hanging promises.Related issue:
Fixes #878
Check list: