CODEWITHSUNDEEP
rust
vasily
vasily

Reputation: 2920

Read file content atomically

On Linux, if there's a long running process that writes a timestamp to a text file every second, how do I read the timestamp from this file atomically?

This is my current implementation:

fn read_timestamp() -> Result<String, &'static str> {
    let path = "/timestamp.txt";
    let metadata_result = fs::metadata(path);
    match metadata_result {
        Ok(_) => {
            let lock_result = FileLock::lock(path, true, true);
            match lock_result {
                Ok(mut lock) => {
                    let mut content = String::new();
                    let read_result = lock.file.read_to_string(&mut content);
                    if read_result.is_ok() {
                        Ok(content)
                    } else {
                        Err("Cannot read file")
                    }
                },
                Err(_) => Err("Cannot acquire lock")
            }
        },
        Err(_) => Err("File not found")
    }
}

It looks clunky, I would appreciate some feedback.

Upvotes: 2

Views: 205

Answers (1)

Masklinn
Masklinn

Reputation: 42622

The one big issue I can see is that the locking is racy, this doesn't actually cause issues but will cause unnecessary and unhelpful syscall traffic: technically the file can be removed between the calls to metadata and FileLock::lock.

So I'd probably just perform the FileLock::lock call directly, then inspect the contents of its error for error-reporting, on failure it returns an io::Error so should tell you that it errored specifically because the file did not exist.

An other thing I find somewhat odd is your reliance on match, especially given how little you use the errors.

I'd tend to use higher-level combinators, or if let for unary or binary code paths (especially when ignoring the payload of the fallback). It's also not really customary to create a local variable only to immediately move it to a match, you would usually not bother with the local. YMMV though, it's certainly not wrong per-se, it's just noisier than I think is necessary or warranted.

This would also probably benefit from a bespoke Error type, avoiding the explicit errors and matching. Anyway here's a possible version without a custom error type:

fn read_timestamp2() -> Result<String, &'static str> {
    let path = "/timestamp.txt";
    match FileLock::lock(path, true, true) {
        Ok(mut lock) => {
            let mut content = String::new();
            lock.file.read_to_string(&mut content)
                .map(|_| content)
                .map_err(|_| "Cannot read file")
        },
        Err(e) if e.kind() == NotFound => Err("File not found"),
        Err(_) => Err("Cannot acquire lock")
    }
}

You could lean even further into the combinators:

fn read_timestamp() -> Result<String, &'static str> {
    let path = "/timestamp.txt";
    FileLock::lock(path, true, true)
        .and_then(|mut lock| {
            let mut content = String::new();
            lock.file.read_to_string(&mut content)
                .map(|_| content)
        })
        .map_err(|e| match e.kind() {
            NotFound => Err("File not found"),
            Interrupted | PermissionDenied | WouldBlock
                => Err("Cannot acquire lock"),
            _ => Err("Cannot read file")
        })
    }
}

though note that this code is not really correct in its error reporting: ErrorKind is rather imprecise, so in reality you'd need to call raw_os_error and cross-reference the man-pages to see what can come from where.

The second verion's a bit wanky to be honest.

Upvotes: 2

Related Questions