Skip to content

Add timeout to tcp connections #2231

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.

Sign up for GitHub

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

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

Add timeout to tcp connections #2231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a206a90
Add timeout to tcp connections
Systemcluster Jul 22, 2024
7296b2b
Add documentation for connection timeout
Systemcluster Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/Configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@Xuanwo Xuanwo Aug 7, 2024

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

@Systemcluster Systemcluster Aug 12, 2024

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?

Copy link
Collaborator

@drahnr drahnr Feb 25, 2025

Choose a reason for hiding this comment

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

It depends, I'd say just the timeout is enough. If we start to see a need for a lot more config values, sub-structs might do as well.

Copy link
Collaborator

@ahartmetz ahartmetz Oct 16, 2025

Choose a reason for hiding this comment

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

I think SCCACHE_CONNECT_TIMEOUT is better. The interpretations that first come to mind for me are:

  • SCCACHE_CONNECT_TIMEOUT: the timeout for making a connection
  • SCCACHE_CONNECTION_TIMEOUT: the timeout for an existing connection

* `SCCACHE_STARTUP_NOTIFY` specify a path to a socket which will be used for server completion notification
* `SCCACHE_MAX_FRAME_LENGTH` how much data can be transferred between client and server
* `SCCACHE_NO_DAEMON` set to `1` to disable putting the server to the background
Expand Down
17 changes: 16 additions & 1 deletion src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@ use byteorder::{BigEndian, ByteOrder};
use retry::{delay::Fixed, retry};
use std::io::{self, BufReader, BufWriter, Read};
use std::net::TcpStream;
use std::time::Duration;
use std::{env, net::ToSocketAddrs};

const DEFAULT_CONNECTION_TIMEOUT: u64 = 20;
Copy link
Contributor Author

@Systemcluster Systemcluster Jul 22, 2024

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.

Copy link
Collaborator

@ahartmetz ahartmetz Oct 16, 2025

Choose a reason for hiding this comment

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

Something like 5, 8, or 10 seems appropriate IMO. No connection between any two computers takes that long unless there is very heavy bufferbloat, packet loss, or an interplanetary connection.


/// Get the time clients should try to connect to the server before continuing, in seconds.
pub(crate) fn get_connection_timeout() -> u64 {
env::var("SCCACHE_CONNECTION_TIMEOUT")
.ok()
.and_then(|s| s.parse().ok())
Copy link
Collaborator

@drahnr drahnr Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think warning on an invalid value as early as possible would be good.

.unwrap_or(DEFAULT_CONNECTION_TIMEOUT)
}

/// A connection to an sccache server.
pub struct ServerConnection {
Expand Down Expand Up @@ -65,7 +77,10 @@ impl ServerConnection {
/// Establish a TCP connection to an sccache server listening on `port`.
pub fn connect_to_server(port: u16) -> io::Result<ServerConnection> {
trace!("connect_to_server({})", port);
let stream = TcpStream::connect(("127.0.0.1", port))?;
let stream = TcpStream::connect_timeout(
&("127.0.0.1", port).to_socket_addrs()?.next().unwrap(),
Duration::from_secs(get_connection_timeout()),
)?;
ServerConnection::new(stream)
}

Expand Down
4 changes: 2 additions & 2 deletions src/dist/client_auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use tokio::runtime::Runtime;
use url::Url;
use uuid::Uuid;

use crate::errors::*;
use crate::{client::get_connection_timeout, errors::*};

// These (arbitrary) ports need to be registered as valid redirect urls in the oauth provider you're using
pub const VALID_PORTS: &[u16] = &[12731, 32492, 56909];
Expand Down Expand Up @@ -503,7 +503,7 @@ async fn try_bind() -> Result<HyperBuilderWrap> {
.expect("Expected at least one address in parsed socket address");

// Hyper binds with reuseaddr and reuseport so binding won't fail as you'd expect on Linux
match TcpStream::connect(addr) {
match TcpStream::connect_timeout(&addr, Duration::from_secs(get_connection_timeout())) {
// Already open
Ok(_) => continue,
// Doesn't seem to be open
Expand Down