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: early exit process so node doesn't wait for hanging promises #182

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

chirag-droid
Copy link
Contributor

Issue

fixes: #181

Proposed Changes

Since Node waits for hanging http requests we early exit by calling process.exit. This doesn't affect the existing API but we can specify optional parameter earlyExit that would apply this fix.

This is related to nodejs/node#47228

@chirag-droid chirag-droid changed the title fix: early exit process so node doesn't want for hanging promises fix: early exit process so node doesn't wait for hanging promises Jan 29, 2024
@chirag-droid
Copy link
Contributor Author

oh my... i misspelled wait to want

@echoix
Copy link

echoix commented Apr 17, 2024

Would it be possible to reconsider merging this PR? Rebasing the changes made on main before?

I'm trying to add this action to save a bit of time in CI, but I'm loosing more time in the post step than the little time saved + overhead

@hendrikmuhs
Copy link
Owner

As said in the issue: I happily merge it once the test-failures are resolved. If @chirag-droid lost interest, maybe you can take over?

@chirag-droid
Copy link
Contributor Author

As said in the issue: I happily merge it once the test-failures are resolved. If @chirag-droid lost interest, maybe you can take over?

I can take up this issue again. I am diagnosing the failed tests

@chirag-droid
Copy link
Contributor Author

image

https://github.com/hendrikmuhs/ccache-action/actions/runs/7697481224/job/22066139179?pr=182

I need help with this test case. And why it's failing.

@mweberxyz
Copy link

mweberxyz commented Apr 18, 2024

@chirag-droid not sure about that exact failure, but process exit codes should be positive numbers. 0 means success, non-0 for failure. Generally, process.exit(1) is what you are looking for in the error handlers.

src/save.ts Outdated
Comment on lines 75 to 77
// A failure to save cache shouldn't prevent the entire CI run from
// failing, so do not call setFailed() here.
core.warning(`Saving cache failed: ${error}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to this comment, should I return 0 even if the caching fails

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I think so, this change was introduced in #59

@hendrikmuhs
Copy link
Owner

Thank you @chirag-droid. The 1 failing test doesn't matter. I can fix that manually. I will merge this PR in the afternoon.

@hendrikmuhs hendrikmuhs merged commit 550708f into hendrikmuhs:main Apr 19, 2024
40 of 41 checks passed
@junaire
Copy link

junaire commented Apr 20, 2024

Do we need to make a new release? @hendrikmuhs

@Naros
Copy link

Naros commented Apr 25, 2024

Is there an update on when we can expect this to be released @hendrikmuhs ?

@njzjz
Copy link

njzjz commented Apr 27, 2024

This is a workaround for me before a new version is released:

    - uses: hendrikmuhs/ccache-action@ff9f6cc67d49b9acaa9e102f80d1ea8a0c86a6dd

@hendrikmuhs
Copy link
Owner

sorry, didn't had time to make a release. Will do it now.

compnerd pushed a commit to compnerd/ccache-action that referenced this pull request Jun 6, 2024
…ndrikmuhs#182)

 fix: early exit process so node doesn't want for hanging promises
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.

Node 20 appears to cause major slowdown in Post ccache step
7 participants