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

Make timeout for hashFiles() configureable #1844

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Carsten-MaD
Copy link

@Carsten-MaD Carsten-MaD commented Apr 22, 2022

As reported here #1840, in scenarios where a lot of files need to be hashed to build up a cache, the process running hashFiles() may run into the fixed 120 second timeout and be cancelled prematurely.

This pull request introduces a new parameter --timeout= that is meant to be passed the same way as the already existing optional parameter --follow-symbolic-links.

Example configuration for actions/cache:

- uses: actions/cache@v3
  with:
    key: ${{ runner.os }}-${{ hashFiles('--timeout=600', '**') }}

Points of discussion:

  • It may be controversial to pass a parameter that affects the timeout of a process like this, as it is not directly related to the hashFiles() command itself, but rather to how it is run. If you see an alternative way of injecting such a parameter, please let me know.
  • For this, I removed firstParameter and the connected logic. Instead, the logic now looks for if a parameter name starts with
    -- and tries to match one of the two expected parameters. Please let me know if you strongly feel the logic that these parameters need to be first is needed, I will then put something in.
  • There are two different exceptions thrown for when the timeout number could not be parsed (e.g. a string passed), and when it was empty. This could be made into one more generic exception to handle and explain both.

- Removed checking if paramter starting with '--' is the first parameter
@Carsten-MaD
Copy link
Author

Not entirely sure, is there anything I need to do so the PR is reviewed, am I missing a step?

@TingluoHuang sorry if I tag you directly, maybe you can advise?

JJ
JJ previously approved these changes Jul 20, 2022
Copy link

@JJ JJ left a comment

Choose a reason for hiding this comment

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

Just a typo.

src/Runner.Worker/Expressions/HashFilesFunction.cs Outdated Show resolved Hide resolved
@Carsten-MaD Carsten-MaD dismissed stale reviews from ghost and JJ via c89c66d August 15, 2022 08:16
@shubhank008
Copy link

This Pr is really needed for us large gamedev project users, any update on this ?

@gotchipete
Copy link

This Pr is really needed for us large gamedev project users, any update on this ?

Agree! (realizing I'm replying to a comment from 10 mo ago)

@haoming29
Copy link

Any updates for this issue or this should be closed? We caught the same error and suspect our runner fails because we have large files to hash and it could go over 120s in many cases.

Copy link

@BE-Digital-Solutions-Bart BE-Digital-Solutions-Bart left a comment

Choose a reason for hiding this comment

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

Code seems good
This merge should solve issue #1840

@alb3rtuk
Copy link

Hey guys, we're running into the same issue with the 120 second hashFiles() timeout. Can we merge this please?

@Propagant
Copy link

So what's the status on this?

@AryanJ-NYC
Copy link

Bump?

@guicaulada
Copy link

guicaulada commented Oct 22, 2024

Running into this as well, it's weird, with 4 vCPU it runs fine, with 32 vCPU (on a very large runner for extra heavy tasks) it fails, if I run the same task on a smaller runner, it succeeds, but the whole workflow takes much longer.

@Karsto-sys
Copy link

also would like to have this

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.