Implement timeouts and retries logic #75
Conversation
| @@ -14,6 +14,12 @@ inputs: | |||
| workspaces: | |||
| description: "Paths to multiple Cargo workspaces and their target directories, separated by newlines" | |||
| required: false | |||
| maxRetryAttempts: | |||
There was a problem hiding this comment.
| maxRetryAttempts: | |
| max-retries: |
Maybe? To rather use dashed names to be consistent wtih the other options.
| : new Promise<never>(() => {}); | ||
|
|
||
| const timeoutSym = Symbol("timeout" as const); | ||
| const racingTimeout = timeout.then(() => timeoutSym); |
There was a problem hiding this comment.
| const racingTimeout = timeout.then(() => timeoutSym); | |
| const racingTimeout = timeout.then(() => { throw new TimeoutError("operation timeout"); }); |
This way you shouldn’t need the symbol and explicit check, just return await Promise.race
| const restoreKey = await withRetries( | ||
| () => | ||
| withTimeout( | ||
| () => cache.restoreCache(config.cachePaths, key, [config.restoreKey]), | ||
| config.timeout | ||
| ), | ||
| config.maxRetryAttempts, | ||
| () => true | ||
| ); |
There was a problem hiding this comment.
The composition is nice, but the construct still looks very unweildy looking at it, I wonder if you can package that up into a single fn?
There was a problem hiding this comment.
Thank you for making this! I am very interested in seeing this merged. Happy to take of the PR if you are busy @MOZGIII!
| /** The max timeout for the networking operations */ | ||
| public timeout: null | number = null; | ||
| /** The max retry attemtps for the networking operations */ | ||
| public maxRetryAttempts: number = 0; |
There was a problem hiding this comment.
Is 0 a useful default? Can we set this to 1 instead?
| /** The max timeout for the networking operations */ | ||
| public timeout: null | number = null; |
There was a problem hiding this comment.
Same here, I think a default value would be useful.
Typically, GitHub downloads the cache with > 100MB/s. Considering that the cache limit is 10GB per repository, downloading a cache shouldn't take much longer that 100 seconds. What about a default value of 2 minutes?
| export async function withRetries<T>( | ||
| operation: () => Promise<T>, | ||
| maxRetryAttempts: number, | ||
| isRetriable: (error: unknown) => boolean |
There was a problem hiding this comment.
The abstraction is nice but we pass () => true here in both usages so perhaps keep it lean and remove that?
| await withRetries( | ||
| () => | ||
| withTimeout( | ||
| () => cache.saveCache(config.cachePaths, config.cacheKey), | ||
| config.timeout | ||
| ), | ||
| config.maxRetryAttempts, | ||
| () => true | ||
| ); |
There was a problem hiding this comment.
In my experience, saving the cache never failed. Caches are also a lot less often saved compared to being restored so hitting rate limits / getting bandwidth-limited is likely a lot less frequent.
I do understand though that we might want to keep this for consistencies' sake.
|
I forgot about this one, @thomaseizinger please do |
I am currently trialing a different solution: libp2p/rust-libp2p#3376 |
|
FYI, https://github.com/actions/cache now includes timeouts by default (and they are configurable by environment variables). |
Closes #72.