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

Add timeout to tcp connections #2231

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

Conversation

Systemcluster
Copy link
Contributor

Depending on system configuration, waiting for a connection error when checking for a running server can take a long time. In my local WSL installation, this is causing sccache to take several minutes to start up.

This PR replaces the two calls to connect with connect_timeout and adds a SCCACHE_CONNECTION_TIMEOUT environment variable for configuration. The code for reading the env var is copied from server.rs:

sccache/src/server.rs

Lines 94 to 101 in 9958e7c

/// Get the time the server should idle for before shutting down, in seconds.
fn get_idle_timeout() -> u64 {
// A value of 0 disables idle shutdown entirely.
env::var("SCCACHE_IDLE_TIMEOUT")
.ok()
.and_then(|s| s.parse().ok())
.unwrap_or(DEFAULT_IDLE_TIMEOUT)
}

use std::time::Duration;
use std::{env, net::ToSocketAddrs};

const DEFAULT_CONNECTION_TIMEOUT: u64 = 20;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be much lower, but leaving it this high aligns it more closely with the current behavior. I'm running it with 2 locally.

@codecov-commenter
Copy link

codecov-commenter commented Jul 22, 2024

Codecov Report

Attention: Patch coverage is 30.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 40.88%. Comparing base (0cc0c62) to head (7296b2b).
Report is 68 commits behind head on main.

Files Patch % Lines
src/client.rs 33.33% 2 Missing and 4 partials ⚠️
src/dist/client_auth.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2231      +/-   ##
==========================================
+ Coverage   30.91%   40.88%   +9.96%     
==========================================
  Files          53       55       +2     
  Lines       20112    20675     +563     
  Branches     9755     9799      +44     
==========================================
+ Hits         6217     8452    +2235     
- Misses       7922     8093     +171     
+ Partials     5973     4130    -1843     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Systemcluster
Copy link
Contributor Author

@Xuanwo could you review this? I don't think it's a controversial change, but I can revert to calling connect without timeout if the environment variable isn't set otherwise.

Copy link
Collaborator

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks and sorry for the late, I only have some small questions.

@@ -129,6 +129,7 @@ configuration variables
* `SCCACHE_CONF` configuration file path
* `SCCACHE_CACHED_CONF`
* `SCCACHE_IDLE_TIMEOUT` how long the local daemon process waits for more client requests before exiting, in seconds. Set to `0` to run sccache permanently
* `SCCACHE_CONNECTION_TIMEOUT` how long clients should try to connect to the server before continuing, in seconds.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall, this idea looks good to me. However, I think it should be a config value in the [dist] section instead of an environment variable. Most distribution configurations aren't exposed to the environment for now.

The current name is a bit confusing from my point of view. I would expect something like tcp_connect_timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should connect_to_server in src/client.rs and the functions that use it (connect_or_start_server in src/command.rs) take the config struct as an argument?

Xuanwo

This comment was marked as duplicate.

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.

3 participants