Std::fs::TempDir ignoring errors on drop

[skade]

It occurred to me while reading the documentation of TempDir::close()1 that:

Although TempDir removes the directory on drop, in the destructor any errors are ignored. To detect errors cleaning up the temporary directory, call close instead.

First of all, the documentation is in the wrong place - the behavior of drop should be described along with drop. My main issue is the behavior though: If I let Rust manage memory as it should and would, it will silently eats errors, unless I explicitly opt in. Errors should always be opt-out in my opinion.

In my opinion, it should at least log, if not even panic! in such an event, notifying of the option to explicitly close() to handle.

Is this a general mode of working for std or is the problem being addressed otherwise?

[pnkfelix]

Note that calling panic! from within a destructor is bad practice.

See discussion here: https://github.com/rust-lang/rust/issues/14875 (note that panic! was named fail! in the context of that discussion).

In particular, the team is explicitly leaving open the option of outright aborting on a panic! within a destructor.

[bluss]

I think this is an example of a destructor that should panic!() on error. What it should do is of course, panic!() only if not already panicking, in which case it should log the error. This is a situation we need a general solution to – for example a function to call!

[mzabaluev]

There is std::thread::panicking that can be used to prevent nested panic. Logging is another matter – it may panic as well. The least the destructor should do is to ensure that the OS resources are freed in all circumstances.

[gkoz]

I’m wondering if debug_assert should just always avoid double panics…

[tbu]

I believe silencing an error is similarly bad practice.

The correct way of solving it is probably providing a panicking destructor and a close method that can return an error (and thus leaving it to the caller whether to “ignore and log” the error).

[llogiq]

From a programmer’s perspective, in at least 80% of all cases, the programmer will probably ignore the error anyway and would be annoyed if the program panics, just because the temp dir couldn’t be dropped.

On the other hand, there may be valid reasons for wanting to be sure that a temp dir has been correctly dropped (i.e. removed), which even might have security implications. However, exiting with a panic seems to be an unfortunate way to handle this.

Perhaps allowing the programmer to set a handler for the case that dropping fails would enable programs to inform the user that something is amiss. POSIX traditionally has signal handlers for this kind of situation.

[skade]

I wouldn’t agree. Just because it is a temp path doesn’t mean that there should be any less scrutiny in working with it as a resource. A resource handling error is a resource handling error. This is also just an example, similar APIs will face the same problem.

The temp dir not being dropped might also be the first indicator to another problem actually leading to that.

Rust does a lot of effort to not allow sloppiness - e.g. through the unused result lint, so dropping these errors as unnecessary seems weird.

[llogiq]

Following the “Rust way”, there should be a zero-cost safe solution to that problem.

But: Panicking the task isn’t it: Let’s see what will happen: Programmer uses temp dir, task panics at temp dir drop, programmer just sees task panic in drop — hopefully debug info is enabled. Probably the panic won’t happen regularly, so it’ll be a heisenbug.

Now we have two options:

• The programmer, perhaps due to lack of experience or due to in unimportance of the data in the temp dir, just leaves it that way. Now we have the worst of both worlds: An insecure drop operation and a randomly failing program.

• The programmer is savvy (and cares) enough to create a watchdog that catches the panicking task, and…what? The temp dir handle is gone, and we have no easy way of retrieving it. So I believe there should be a better way.

My proposed solution is to allow the programmer to install a handler that can take care of any temp dir drop failure. This would still be better than the above option, because at least the program runs to completion, and in the second option give the programmer more information to handle the failure. If you really like it, make a handler function part of the signature of allocating a temp dir.

[kennytm]

I don’t think a handler function works if what I want is just to propagate the error, instead of ignoring it or resolving it immediately.

// Propagation of error works fine with explicit close()
fn handle_error_with_close() -> Result<()> {
    let dir = try!(TempDir::new("tmp"));
    ...
    try!(do_stuff(&dir));
    ...
    try!(dir.close());
    Ok(())
}

// Cannot propagate the error upward
fn handle_error_with_callback() -> Result<()> {
    let dir = try!(TempDir::new("tmp"));
    dir.trap_drop_failure(|e| what_to_do_here?(e));
    ...
    try!(do_stuff(&dir));
    ...
    Ok(())
}

[llogiq]

In this case, you would need a closure that closes over a mutable value of the containing function.

[kennytm]

So it would need to look like this

fn handle_error_with_callback() -> Result<()> {
    let mut return_value = Ok(());
    {
        let dir = try!(TempDir::new("tmp"));
        dir.trap_drop_failure(|e| return_value = Err(e));
        ...
        try!(do_stuff(&dir));
        ...
    }
    return_value
}

[llogiq]

Yes. As I’ve stated in another thread, this looks needlessly complex and certainly not rusty.

Therefore, while it would have enabled a few cool things, I’m going to retract my proposal, as I’ve done in that other thread.

Linear types would afford a solution, but they certainly impose some implementation cost.

[system]

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