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.

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

[commands::run_server_process]: fix race condition #2077

Merged
merged 6 commits into from
Feb 7, 2024
Merged

[commands::run_server_process]: fix race condition #2077

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c80c389
[commands::run_server_process]: fix race condition
arvidj Feb 4, 2024
3d9ad9b
fixup! return error from bind directly
arvidj Feb 5, 2024
8896b35
fixup! fmt
arvidj Feb 5, 2024
b494bf9
fixup! explain race avoidance
arvidj Feb 5, 2024
ec2f336
fixup! simply code for listener
arvidj Feb 6, 2024
b23a0a5
fixup! remove redundant clone()
arvidj Feb 7, 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
8 changes: 7 additions & 1 deletion src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,13 @@ fn run_server_process(startup_timeout: Option<Duration>) -> Result<ServerStartup
let runtime = Runtime::new()?;
let exe_path = env::current_exe()?;
let workdir = exe_path.parent().expect("executable path has no parent?!");

// Spawn a blocking task to bind the Unix socket. Note that the socket
// must be bound before spawning `_child` below to avoid a race between
// the parent binding the socket and the child connecting to it.
let sp = socket_path.clone();
let listener = runtime.block_on(async move { tokio::net::UnixListener::bind(sp) })?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be written let listener = tokio::net::UnixListener::bind(&socket_path.clone())?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It compiles, but when I run it I get:

$ ./target/debug/sccache --start-server
sccache: Starting the server...
thread 'main' panicked at 'there is no reactor running, must be called from the context of a Tokio 1.x runtime', src/commands.rs:98:20
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The way I understand it: the function run_server_process is not async. Therefore, you cannot call async functions in it without starting a Tokio runtime(?). tokio::net::UnixListener::bind is an async function. Therefore, you must call it inside a runtime.block_on call so that it runs in a runtime(?).

Copy link
Collaborator

Choose a reason for hiding this comment

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

The function is not, actually, async. https://docs.rs/tokio/latest/tokio/net/struct.UnixListener.html#method.bind But it does not want to run without a runtime.
So the following would work too, without an async block:

let listener = {
    let _guard = runtime.enter();
    tokio::net::UnixListener::bind(&socket_path.clone())?
};

Copy link
Contributor Author

@arvidj arvidj Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed. I've pushed a fixup with your last suggestion.


let _child = process::Command::new(&exe_path)
.current_dir(workdir)
.env("SCCACHE_START_SERVER", "1")
Expand All @@ -97,7 +104,6 @@ fn run_server_process(startup_timeout: Option<Duration>) -> Result<ServerStartup
.spawn()?;

let startup = async move {
let listener = tokio::net::UnixListener::bind(&socket_path)?;
let (socket, _) = listener.accept().await?;

read_server_startup_status(socket).await
Expand Down
Loading