Add filename information to io::Error

[zackw]

Consider a toy program that creates files:

use std::env;
use std::fs::File;

fn main() {
    let fname = env::args().skip(1).next().unwrap();
    let _ = File::create(fname).unwrap();
}

If this program fails to create a file …

$ ./toy_create /etc/foo
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value:
  Error { repr: Os { code: 13, message: "Permission denied" } }',
  /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libcore/result.rs:868

… the error message you get does not include the name of the file that it failed to create.

This program could, of course, have been written to print its own error messages that do include the name of the file. But imagine a more complicated situation, where you call into some library and get that io::Result back, and you don’t know the name of the file it was trying to access, or even if it was trying to access a file.

(Elder Days Story Time: ten years ago, one of the most common user complaints about monotone was that it would print “Permission denied” with no further detail if it didn’t have write access to its database or the directory containing it. We couldn’t improve the error message because all we got back from the SQLite library was an errno code; we knew the name of the main .sqlite file, but not the names of the temporary files the library would use, or which one was the problem.)

So I would like to suggest that io::Error be enhanced with additional fields, holding the name(s) of the file(s) involved in the failed operation, and the name of the system call that failed. Python 3’s OSError exception, which serves a very similar function to io::Error, has filename and filename2 fields; it doesn’t capture the name of the failing system call, but I rather think it ought to.

[WiSaGaN]

This program could, of course, have been written to print its own error messages that do include the name of the file. But imagine a more complicated situation, where you call into some library and get that io::Result back, and you don’t know the name of the file it was trying to access, or even if it was trying to access a file.

In that case, just make a pull request to that library to include the necessary information in the error.

[zackw]

… just make a pull request to that library …

That doesn’t scale. Adding the information to io::Error fixes the problem in one place for everyone (as long as libraries follow good error-chaining practice).

[WiSaGaN]

I don’t think trying to change a language whenever you find some badly implemented library is going to scale either…

[withoutboats]

To me this idea seems reasonable, but I’m not sure its in line with the philosophy of the standard library. In general, it’s trying to provide a very minimal wrapper around what the OS provides. This would involve adding an Option<PathBuf> to io::Error, which is a bit ‘heavier’ than what std normally does.

[kornel]

I’ve ran into this too, and I wish the path was included.

It’s a pain for me both as a user of crates (since I get vague errors from build.rs scripts) and as an author (since I can’t just propagate the std error, I have to make my own wrapper, which I’m too lazy to do, so my programs are bad at error reporting too).

[alexcrichton]

This was historically done and removed during the I/O reform leading up to 1.0. I didn’t find any references as to why, but I recall discussing this aspect and it was chosen to not do so in libstd as it can obfuscate the underlying raw error coming out of the syscall. The expectation is that crates outside of std would typically do this, but std itself would avoid doing so.

Another downside of doing this in std is that it can be inefficient. Operations which expect a large number of failures currently don’t allocate any extra data for the error path, but with a change such as this they’d have to allocate both the error itself and the PathBuf to store inside.

[repax]

I don’t think it’s a good idea to mandate the inclusion of data or operations when they’re not universally applicable. Let’s go with the small footprint instead. A user library could include this information if it’s relevant in its niche. Not all applications have this need, they might not even have a terminal or textual log file.

[zackw]

I don’t think it’s a good idea to mandate the inclusion of data or operations when they’re not universally applicable.

I categorically reject this position. Based on many, many years of bitter experience debugging shit in production: All applications, no matter what they are or what environment they are in, need the ability to capture and log system call failures in full detail, somehow. Anyone who thinks they don’t is wrong.

The expectation is that crates outside of std would typically do this, but std itself would avoid doing so.

Other people have expressed essentially the same sentiment, but it has been my experience (from other languages) that the “crates outside of std” won’t bother; if this information is not captured right at the point where the system call happens, it will be lost forever. So I really do think std needs to take care of this.

Operations which expect a large number of failures currently don’t allocate any extra data for the error path, but with a change such as this they’d have to allocate both the error itself and the PathBuf to store inside.

This is a legitimate concern and I don’t immediately see how to thread the needle. To take a concrete example, right now DirBuilder::create_dir_all contains a TOCTOU race. A correct implementation would be something like

fn create_dir_all(&self, path: &Path) -> io::Result<()> {
    if path == Path::new("") { Ok(()) }
    else if let Err(e) = self.inner.mkdir(path) {
        match e.kind {
            io::ErrorKind::AlreadyExists => Ok(()),
            io::ErrorKind::NotFound => {
                if let Some(p) = path.parent() {
                    self.create_dir_all(p)?
                    self.inner.mkdir(path)
                } else {
                    Err(e)
                }
            },
           _ => Err(e)
       }
    }
}

To avoid wasteful alloc-free cycles, we would want to capture the path argument to either call to inner.mkdir if and only if the Err is going to escape this function. Automagically. I believe it’s possible, but it would be some pretty serious optimization magic, especially in the presence of recursive calls, as here. (Note also that optimal logging would capture both the original argument to DirBuilder::create and the specific path that mkdir horked on.)

However, the above code should also, I hope, demonstrate why libraries in general cannot be expected to do the capturing themselves; a fully instrumented version of this function would be nearly twice as long. So, again, I really do want to find an automatic solution somehow.

[kornel]

To avoid allocations, could the error expose a field of type Option<PathBuf> or similar, and set it only when the operation owns the path? (e.g. File::open(path), but not File::open(&path)).

[repax]

I categorically reject this position.

Not much use in discussing your proposal then if you outright reject any position not in support?

All applications, no matter what they are or what environment they are in, need the ability to capture and log system call failures in full detail, somehow.

• Not including the filename does not dissolve the possibility of capturing and logging system call failures. You’re making a false dichotomy.

• Not including the filename does not prevent you from keeping a reference to it yourself. Usually, the context alone is enough information. Otherwise, the caller can simply keep a reference to it.

Let the programmer decide what to pay for.

[zackw]

To avoid allocations, could the error expose a field of type Option<PathBuf> or similar, and set it only when the operation owns the path? (e.g. File::open(path), but not File::open(&path)).

That seems like it would lead to people writing higher-level code constantly being unsure whether paths would get logged or not. Might actually be worse than not having it at all.

Not much use in discussing your proposal then if you outright reject any position not in support? … Let the programmer decide what to pay for.

Well, I really am rejecting the “let the programmer decide” position out of hand, but I don’t think that means there’s no use discussing it. I mean, I’m the one asking for a change here, I have to persuade you that the programmer shouldn’t be allowed to decide.

The trouble with “let the programmer decide” is that, and this is 20 years of bitter experience talking, every time a programmer thinks they don’t need to report the offending file name(s) when a system call fails, they are wrong. D. Richard Hipp thought that users of SQLite didn’t need to know whether a database open failed because the user didn’t have read access to the main database file, or because they didn’t have write access to the journal file. He was wrong. Any number of people writing Makefiles have thought it was OK to hide the full compiler command lines, because nobody needs to debug a build robot that they don’t have direct access to, using only the log files, with a multi-day turnaround for modifications. They are wrong. At least two undergraduates every year think they don’t need to print out the file name when fopen fails, and I tell them otherwise, and they are dumbfounded to discover that the problem is obvious once they do that.

And because programmers have this tendency to think, incorrectly, that the filename isn’t always a necessary piece of information, crate authors can’t be trusted to capture it.

(Honestly, one of the things I don’t like about “everyone makes up their own error type” is that I don’t trust crate authors to chain in an underlying io::Error every single time either. I want it to be a compile-time failure if you don’t.)

Not including the filename does not prevent you from keeping a reference to it yourself. Usually, the context alone is enough information. Otherwise, the caller can simply keep a reference to it.

This is true when you are only one or two levels up the call stack, but it is not true when you have crossed an abstraction boundary. Look again at the DirBuilder::create_dir_all example I gave: when that fails, the application knows the path it passed in, but it doesn’t know the path that actually provoked an error from the operating system, which could be any prefix of the input path.

[troplin]

Errors are not primarily meant for logging or debugging, but for error handling. It is often argued, that error handling is allowed to be slow because it only happens infrequently. But that’s not always the case and for example in C# this has lead to API duplication (e.g. Dictionary<T>.TryGetValue).

IMO error types should contain only the minimal information necessary in the context of the immediate caller. Sure, that means the caller might need to add some additional context in some cases. And it also means that not every programmer will do it. But this is a standard library. You cannot make any assumptions about how and where it is used. If it is possible to use it in a certain way, it will probably be used like that.

By the same reasoning you could also argue that HashMap::get() should not return an None but an error that contains the requested key value. Or even that all function calls that can fail somehow should return an error containing all of its parameters. There’s always someone that misses that exact information. I don’t think that’s reasonable.

This is one reason why I don’t like exceptions. The main advantage of exceptions is, that you can propagate them so easily over multiple levels, especially if they are not checked. This leads to a programming style that just catches all exceptions at the top level and logs them. Of course at the top level there’s not enough context to actually handle them, and not enough information in the log to find the cause. But that’s not something that should be encouraged IMO. Errors should be handled where they occur, and if that’s not possible, a higher level error should be propagated.

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.