Fix possible deadlock if max_open_files set too low #908

allada · 2024-05-10T21:14:29Z

We now close the file permit if the write future takes too long.

fixes #907

This change is

CLAassistant · 2024-05-10T21:14:34Z

All committers have signed the CLA.

allada

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Bazel Dev / ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, vale, and 1 discussions need to be resolved (waiting on @adam)

nativelink-util/src/lib.rs line 39 at r1 (raw file):

/// Initialize tracing.
pub fn init_tracing() -> Result<(), nativelink_error::Error> {

I did this to help debug tests, so figured I'd sneak it in.

adam-singer

Reviewed 14 of 15 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and 1 discussions need to be resolved

nativelink-macro/src/lib.rs line 40 at r2 (raw file):

            let _ = nativelink_util::init_tracing();
            // If already set it's ok.
            let _ = nativelink_util::fs::set_idle_file_descriptor_timeout(std::time::Duration::from_millis(100));

nit: Seem like something that should be a parameter defaulted to 100ms but configurable, 150-300ms hopefully should be max budget to complete a request in an ideal scenario

nativelink-store/src/filesystem_store.rs line 805 at r2 (raw file):

            }),
        );
        drop(file);

nit: lets comment on why the drop since we are explicit about releasing the reference within a function

nativelink-store/tests/filesystem_store_test.rs line 1421 at r2 (raw file):

    async fn update_with_whole_file_closes_file() -> Result<(), Error> {
        let mut permits = vec![];
        // Grab al permits to ensure only 1 permit is available.

nit: "Grab al" -> "Grab all"

nativelink-store/tests/filesystem_store_test.rs line 1473 at r2 (raw file):

    async fn update_with_whole_file_slow_path_when_low_file_descriptors() -> Result<(), Error> {
        let mut permits = vec![];
        // Grab al permits to ensure only 1 permit is available.

nit: "Grab al" -> "Grab all"

nativelink-store/tests/filesystem_store_test.rs line 1492 at r2 (raw file):

        let store = Box::pin(FastSlowStore::new(
            // Note: The config is not needed for this test, so use dummy data.
            &nativelink_config::stores::FastSlowStore {

nit: alias or import the struct (applied to same below, if existing code is like that I guess leave)

nativelink-util/src/fs.rs line 372 at r2 (raw file):

        Ok((
            permit,
            std::fs::File::options()

this code makes me happy and sad at the same time.

nativelink-util/src/lib.rs line 42 at r2 (raw file):

    use tracing_subscriber::prelude::*;

    static LOGGING_INITIALIZED: std::sync::Mutex<bool> = std::sync::Mutex::new(false);

nit: use std::sync::Mutex

adam-singer

Reviewable status: 1 of 1 LGTMs obtained, and 1 discussions need to be resolved

We now close the file permit if the write future takes too long. fixes TraceMachina#907

allada

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Local / ubuntu-22.04, Publish image, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04

nativelink-macro/src/lib.rs line 40 at r2 (raw file):