Conventions for checked teardown

[dtolnay]

We have a guideline:

Destructors never fail (C-DTOR-FAIL)

Destructors are executed on task failure, and in that context a failing destructor causes the program to abort.

Instead of failing in a destructor, provide a separate method for checking for clean teardown, e.g. a close method, that returns a Result to signal problems.

Let's provide further guidance around naming the teardown method as well as how to decide whether it accepts self or &mut self.

Relevant tempdir issue: rust-lang-deprecated/tempdir#26

[opilar]

We would need to call close several times in case of tearing down with multiple attemts. I vote for &mut self.

[kamalmarhubi]

Alternatively, the method can take self, and the Err variant can return the value in case of error. There is precedent for that in libstd: String::from_utf8. I think that's preferable to taking &mut self. Otherwise we might need additional state so that drop only attempts to clean up if necessary.

[opilar]

@kamalmarhubi close can return Result and can take &mut self. That does not contradict each other.
String::from_utf8 is opposite to close. It takes the ownership of it's parameter. close does not.

[Kixunil]

@opilar if close would invalidate the object it should take self to prevent accidental use.

[brson]

@dtolnay we discussed this last week and came to some conclusions but I can't remember what they were. I recall it seemed clear cut, but after reviewing this thread it doesn't seem so.

• Taking &mut self makes it so you can retry a close if it fails
• But taking &mut self means that you have to worry about handling all operations while in the closed state, which is quite burdensome
• Take self means you either don't get to retry on failure, or you have to return self in the error type

As with all things dtor seems like it's all bad tradeoffs.

I think it's also reasonable to note that it's not obvious that retrying a failed close makes sense. Failing to close an I/O resource is almost always a nightmare, no-win scenario. I don't think I've ever seen a recommendation to retry closing a thing, and istm that an API that demanded close be retried would be extremely unfriendly. On some systems common wisdom is to assume close succeeds and just ignore failures to close. Having an explicit close in most cases seems primarily an opportunity to do some logging that something is really messed up.

On the other hand in the tempdir case, closing means delete an entire directory tree, and we know that, especially on windows, that is a fraught operation. Still, we've even recently added defensive code in tempdir to do all kinds of things to try to make the delete 'just work', so it's hard to imagine what other heroics we might expect the caller to do to fix our failure to close.

[Kixunil]

I don't think it'd be a good idea to make a "law" in case different implementations might need to do different things. I actually like encoding in type whether operation is retry-able.

The guidelines could be:

• if failed operation doesn't invalidate the resource in any way, take by &mut self and return Result<(), E>
• if failed operation invalidates the resource and can't be retried take bye self and return Result<(), E>
• if failed operation can be retried but makes the resource otherwise unusable, take by self and return Result<(), (E, RetryHandle)>, where RetryHandle is user-defined type with retry method. The retry method should take self and return Result<(), (E, Self)> (note that in this case Self is the RetryHandle)

[opilar]

@Kixunil @brson What to do with an error while an object goes out of scope?

{
    let a = A::new();
} // <-- error here

[Kixunil]

@opilar I think we have to preserve current convention - just ignoring the error.

[opilar]

@Kixunil I think ignoring the error is incorrect.
I think close should return () and the programmer should make a choice to ignore the error or panic.
To retry and get an error we can make another method with Result<(), E> return type.

[Kixunil]

@opilar Drop::drop() should never panic as that leads to abort. There is really no other way to solve this unfortunately. Also at Unix, close can't be retried. If one wants to ensure data is written on disk, he must use fsync. AFAIK after successful fsync it doesn't matter whether close will succeed.

[opilar]

@Kixunil You are right. For general case I think close(&mut self) -> Result<(), E> with ignoring the error is appropriate. A developer can use it to retry the dropping operation if it does make sense.

[Kixunil]

I don't think close(&mut self) -> Result<(), E> is a good idea. Consider this scenario:

• Thread 1 calls close()
• close fails, but OS deletes FD
• Thread 2 calls open(). The returned FD number is the same as previously closed FD
• Thread 1 calls close() again to retry - FD opened by thread 2 is closed
• Thread 2 has now invalid FD that was invalidated by other thread.

Obviously, close() must consume self in order to avoid this. This will become even worse if thread 3 opens another file just after thread 1 retried: threads 2 and 3 will share same FD and likely corrupt data. Finding out the cause would be a nightmare.

[sfackler]

It can replace the file descriptor with -1 on Unix and the handle with INVALID_HANDLE on Windows.

…

On Tue, Jul 11, 2017 at 5:11 AM Martin Habovštiak ***@***.***> wrote: I don't think close(&mut self) -> Result<(), E> is a good idea. Consider this scenario: - Thread 1 calls close() - close fails, but OS deletes FD - Thread 2 calls open(). The returned FD number is the same as previously closed FD - Thread 1 calls close() again to retry - FD opened by thread 2 is closed - Thread 2 has now invalid FD that was invalidated by other thread. Obviously, close() must consume self in order to avoid this. This will become even worse if thread 3 opens another file just after thread 1 retried: threads 2 and 3 will share same FD and likely corrupt data. Finding out the cause would be a nightmare. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#61 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABY2URoP0n8ED_fGONx-sNRsgFI4xZyHks5sM2ZXgaJpZM4Naj6M> .

[Kixunil]

Yes, but that completely defeats the purpose of retrying and may introduce errors that could've been caught compile-time. I like Rust because precisely it can avoid such things. I'd even say that not consuming self is anti-Rust, unless it comes with significant performance hit.

[sfackler]

The runtime errors are going to exist one way or another if the File/whatever lives in a struct.

[Kixunil]

Sure, that doesn't mean we should allow errors being more confusing.

[opilar]

@dtolnay @brson seems like we're stuck here.

[brson]

Clearly no resolution so far for the general case. Maybe there's a resolution for tempdir.

[alexcrichton]

cc rust-lang/rust#32255, the discussion on rust-lang/rust, notably this comment

One other point I think worth mentioning is that I think the ergonomics of &mut self vs self. In some situations self can be quite natural but in others it can be quite difficult to use. For example consider something like:

struct A { /* ... */ }
struct B { /* ... */ }
struct C {
    a: A,
    b: B,
}

impl A {
    fn close(self) -> Result<(), A> { /* ... */ }
}
impl B {
    fn close(self) -> Result<(), B> { /* ... */ }
}

impl C {
    fn close(self) -> Result<(), C> {
        let C { a, b } = self;
        match a.close() {
            Ok(()) => {}
            Err(a) => return Err(C { a, b }),
        }
        match b.close() {
            Ok(()) => Ok(())
            Err(b) => {
                // panic? store an `Option<A>`? different error?
            }
        }
    }
}

Here we've got types like A and B which have a self-style close method, and then we're trying to create a composite resource C which contains both A and B. If we try to implement a self close method on C we get in a situation where:

• The method itself is pretty painful to right. Lots of destructuring, movement, handling of errors, etc.
• We still have to deal with partial close states. Our first close may succeed but the latter close may fail, so we either need to communicate this in our error type or deal with optional state internally.
• We probably need to manufacture a unique error type for this method instead of just returning C, and we'll likely need a unique type per-close, which can get quite wordy when we consider conventional error messages.

When compared to &mut self though:

impl C {
    fn close(&mut self) -> Result<(), MyError> {
        self.a.close()?;
        self.b.close()?;
    }
}

The &mut self version is more ergonomic to write, is the same in the "successful case", and doesn't require new kinds of errors. The downside of this approach, of course, is that a and b may be forced to contain internal optional state. In experience with futures, though, this is actually relatively rare in practice as only the leaf types need this optional state and everything else built on them can rely on optional state elsewhere. For example C here doesn't need any optional state.

This to me drives home the the debate personally, placing me firmly in the &mut self-as-a-recommendation camp.

[brson]

Hm, @alexcrichton's observations about the difficulty of dealing with partially-destructured types in the by-value case is pretty convincing.

[brson]

If &mut self were the guideline here, often forcing implementors to track a bool indicating a resource is closed, I'd hope we can also suggest that the proper response to doing any operation on a closed resources is to panic, e.g. assert!(!self.is_closed()).

If that is true, we might also want to say that it is acceptable to call close multiple times, to deal with retries in the partial-success case. IOW, if you have a closable thing that contains two closable things, like @alexcrichton's previous example, and on the first call to close, one succeeds, and one fails; the user needs to be able to call close again, and the impl for C needs to look like one of the following:

impl C {
    fn close(&mut self) -> Result<(), MyError> {
        self.a.close()?;
        self.b.close()?;
    }
}

impl C {
    fn close(&mut self) -> Result<(), MyError> {
        if !self.a.is_closed() { self.a.close()?; }
        if !self.a.is_closed() { self.b.close()?; }
    }
}

Seems easiest to say that "it is a no-op to call close on a closed resource".

[brson]

Hm, on the other hand, I still think it's basically hopeless to retry closes generally, and I might suggest that a failed close where multiple inner resources fail to close is one of those horrible dtor scenarios where there is no right solution, so I'm still not totally convinced against self.

Instead of @alexcrichton's outline for the multiply-closing C here:

impl C {
    fn close(self) -> Result<(), C> {
        let C { a, b } = self;
        match a.close() {
            Ok(()) => {}
            Err(a) => return Err(C { a, b }),
        }
        match b.close() {
            Ok(()) => Ok(())
            Err(b) => {
                // panic? store an `Option<A>`? different error?
            }
        }
    }
}

Where the first failed close immediately returns, I might suggest that C has a responsibility to attempt to close all internal resources and return whatever errors happened, more like

impl C {
    fn close(self) -> Result<(), MyError> {
        let C { a, b } = self;
        let r_a = a.close();
        let r_b = b.close();
        r_a?;
        r_b?;
        Ok(())
     }
}

That doesn't cover the case where both a and b failed to close of course. Notice though that I'm not returning the self-type in the error - my assumption is that retrying the close is futile and the error should be returned. If retries should be supported then I think &mut self is more ergonomic.

[alexcrichton]

Ah yeah I don't think in general we should strive to support retrying a failed close operation. The close syscall itself you can't retry on a cross-platform basis and lots of scenarios tend to bubble up from that. I'd be more worried about reclaiming resources from a closed object rather than retrying the close operation, though.

One example could be something like a buffered reader where after you close it you want to reclaim the buffer to recycle it (or something like that). In that sense I think that if we recommend self we'll also want to recommend that both variants of the Result carry Self somehow to reclaim those resources. With &mut self though we'd get this for free.

It's true though that I don't know what we would ideally do in the face of partial errors on close. I want to say this is enough:

impl C {
    fn close(&mut self) -> Result<(), MyError> {
        self.a.close()?;
        self.b.close()?;
    }
}

but I fear a "more correct" situation would look like:

impl C {
    fn close(&mut self) -> Result<(), MyError> {
        self.a.close()
            .and(self.b.close())
    }
}

Where in the latter we may drop an error on the floor but in the former we may not actually close resources.

[Kixunil]

Why don't we write helper crate for dealing with these situations instead of throwing array Rust's safety guarantee? Why even use Rust then?

Let's think about how to design multi-close API. For simplicity, assume we have only two resources a, b. Possible options:

• both succeeded
• a failed, b succeeded
• a succeeded, b failed
• both failed

Which data structure should be used for this? Yep, enum! Well, since we want to return Result, here's how to do it:

// This would be in extern crate
enum CloseError<EA, EB> {
    A(EA),
    B(EB),
    Both(EA, EB),
}

// Handy alias
type CloseResult<EA, EB> = Result<(), CloseError<EA, EB>>;

impl<EA, EB> CloseError<EA, EB> {
    fn from_results(a: Result<(), EA>, b: Result<(), EB>) -> CloseResult<EA, EB> {
        match (a, b) {
            (Ok(()), Ok(())) => Ok(()),
            (Err(A(e)), Ok(())) => Err(CloseError::A(e)),
            (Err(B(e)), Ok(())) => Err(CloseError::B(e)),
            (Err(A(ea), Err(B(eb))) => Err(CloseError::Both(ea, eb)),
        }
    }
}

// User would write just this
fn close(self) -> CloseResult<MyErrA, MyErrB> {
    CloseError::from_results(self.a.close(), self.b.close())
}

Using macros we can write CloseError2<A, B>, CloseError3<A, B, C>, CloseError4<A, B, C, D> ...

[sfackler]

It seems bad to break backwards compatibility because you added a new closeable private field to your type.

[Kixunil]

@sfackler what do you mean?

[Ixrec]

@Kixunil I'm guessing he means that any client code that calls close() and then does a match on the CloseError will be broken if you ever add a new variant to the CloseError enum, which you would have to do if you ever added a third thing-to-be-closed inside your type.

This is exactly why I believe "non-exhaustive enums" (rust-lang/rfcs#2008) are a good fit for custom error types.

[Kixunil]

@lxrec Well, that's the problem with strong typing in general: if you are too specific, you may cause backward incompatibility.

[KodrAus]

Is there a distinction between a dropped value and an invalidated one (or as I like to call them: prunes) in general? We recommend leaving builders as prunes instead of consuming self.

EDIT: I guess it's just compile time validation vs runtime validation.

I personally like the idea of the self approach better, but I think the &mut self approach is probably the best recommendation because, as @alexcrichton mentioned, you don't necessarily escape from managing internal state with the self approach. It seems to offer more flexibility as close methods becomes more complex, and that's complexity that you need to navigate carefully. Plus it's more closely aligned with Drop (but that might not actually be important).

ANOTHER EDIT: To clarify, I agree with @Kixunil in principle but think as something we recommend to anyone doing checked teardown the &mut self approach lends itself more naturally to complexities like managing multiple closable resources and idempotent or otherwise retryable teardown.

But if it is too broad to recommend a single approach maybe we could do the same as the builder guideline and discuss the tradeoffs of both?

[alexcrichton]

@KodrAus I think that's actually a very good way to put it, in "simple" cases a by-value self seems to have lots of benefits but a "complex" case is where self starts to show growing pains and &mut may take over.

In the sense of guidelines I'd prefer to make a concrete recommendation one way or another to ensure that the ecosystem works well together. If you, for example, tried to compose by-value self closing methods and by-mut closing methods it'd be pretty painful :(

[Kixunil]

One thing to consider when deciding this is, that taking by self vs taking by &mut self is somewhat similar to returning Result and panicking. The first instance allows the user to choose how error states should be handled (in case of self, the user can opt-in to &mut self semantics with Option), the second one forces opinions on users. (The user can't decide that he prefers compiler checks.)

The fact is that there is no significant difference between self + Option and &mut self. In both cases, there must be a check (there's no way around it). Option is explicit opt-in to panicking (usually used with unwrap) and may be easier to debug (did your code panic on unwrap? search for unwrap()!).

Finally, "what if someone doesn't want to call unwrap() on every use?" That's what DangerousOption is for.

[KodrAus]

@Kixunil I'm not sure I follow what you mean.

As a recommendation for an API that libraries should follow, I'm in favour of &mut self because the ergonomics scale better with complexity. Also, any bugs related to using the resource after it's closed will probably be fairly localised to where it was closed. The risk is that your code panics at runtime, which can be a problem, but isn't unsafe.

If you're building an application and want to make your checked teardown take self by value, which seems like a reasonable thing to do, then you can wrap &mut self teardown methods of internal resources and do that.

[Kixunil]

What I mean is that if one takes by self the user of such thing can decide for themselves how to handle stuff. If they simply want panic semantics, they use unwrap() or DangerousOption. If they are writing a critical code, where panicking should never happen, they can still write their code in such way to ensure it doesn't.

[KodrAus]

@Kixunil I don't quite agree that an &mut self receiver as opposed to a self receiver limits your options for how to handle errors. In either case you'll receive a Result, the difference is whether the error variants includes self that will have a different scope than &mut self.

The kinds of bugs that are possible to result from &mut self that aren't also possible with self are attempting to re-use that prune'd structure after close. My argument is that the likelihood and risk of those bugs (panicking) don't warrant the complexity and loss of composability of a self receiver in libraries.

I think we've exhausted the conversations around ergonomics and compile-time safety and will either have to accept the tradeoffs of one of the approaches or not recommend anything. If we can't recommend anything then I'd suggest simply leaving tempdir's close method as-is (it's currently a self close method).

Do you have any ideas on how to move forward @dtolnay @alexcrichton?

[Kixunil]

@KodrAus It limits one how to handle accidental misuses. Taking by &mut self means it will panic and you can do nothing about it. Taking self means the code is either correct or won't compile.

My argument is that the likelihood and risk of those bugs (panicking) don't warrant the complexity and loss of composability of a self receiver in libraries.

It's not just that. Such design introduces another "invalid" state that must be handled in every method. This doesn't just slow down the code but puts burden on the implementor to write those checks (and not forget them).

What you really want is DangerousOption. It bridges the two worlds and it is sufficiently explicit.

or not recommend anything.

Maybe this is the best approach so far.

[danielpclark]

I prefer &self. In a file scenario you open a file, write to it, and close it. That shouldn't remove the reference to the file and it should still be able to be reopened off of the same &self.

I'd like this for a standard:

trait Close {
  fn close(&self) -> Result<(), E>;
}

Stuff will get dropped out of scope anyways.

As for the example @alexcrichton gave with nested close method calls I believe this could be handled through the Error type. Just as Error has a cause method it could also potentially have a source method. From that instead of using the ? operator to pass the error up the chain there can be a middle-man catch for the error that can deal with the source method. Not to mention the fix for the implementation could also just be written into the close method in the first place should one of the two items fail to close. If that's not the case I would argue that those multiple closes do not belong in one close as that seems to be rather unsafe and should be handled in stages rather than once in close.

In my opinion the added complexity scenarios are more the responsibility of the individual developers rather than a concern for a general standard. Handling situations that come up as you go along is a very normal thing and using close alone, as a standard, to try and solve that is a bit much.

Is it just me or does anyone else mistakenly read close as clone all the time?

[Kixunil]

@danielpclark That's a very bad idea because it allows accidental use-after close. It also requires implementor to keep file name (url, whatever) stored too (allocated on the heap).

[danielpclark]

@Kixunil

That's a very bad idea because it allows accidental use-after close.

That's not necessarily the case. If the code is implemented in a way where something can be “used” only when open then closing it would end the allowed behavior until re-opened again.

It also requires implementor to keep file name (url, whatever) stored too (allocated on the heap).

I don't see that as a bad thing. Drop handles scope cleanup anyways.

Here's How Ruby does Tempfile

require 'tempfile'

file = Tempfile.new('foo')
file.path      # => A unique filename in the OS's temp directory,
               #    e.g.: "/tmp/foo.24722.0"
               #    This filename contains 'foo' in its basename.
file.write("hello world")
file.rewind
file.read      # => "hello world"
file.close
file.unlink    # deletes the temp file

Having the file name after was important in this use case. I imagine unlink would be implemented in Drop in Rust for Tempfile, but there could arguably be a use case where a person doesn't use Drop in their code.

[Kixunil]

@danielpclark the difference is that if you accidentally write something like this:

let f = open("file")?;
f.read(&mut buf)?;
f.close()?;

// some code here

if(som_unlikely_condition()) {
    f.read(&mut buf)?;
}

In your case the code will panic or fail and that will be discovered only during testing. That takes time and it will require the programmer to analyze the situation to understand where the problem is. In my case it won't compile in the first place and the compiler will tell the programmer the exact line where the problem is.

Ruby is not Rust - if it was, we wouldn't have Rust in the first place. Sure, there are cases when keeping the file name around is practical, but that should be the choice of the library user.

[mzabaluev]

I think the compound close example could be improved with Result/Option adapters:

impl C {
    fn close(self) -> Result<(), CCloseError> {
        let ar = self.a.close();
        let br = self.b.close();
        match (&ar, &br) {
            (&Ok(), &Ok()) => Ok(()),
            _ => Err(CCloseError::from_inner(
                        ar.err(),
                        br.err()))
        }
    }
}

The caller should be able to look into CCloseError for the optional inner error data and extract whatever state (which may or may not be the original inner objects) was salvaged from the failed teardowns for graceful error handling. This might expose too much implementation detail, though.

[federicomenaquintero]

Do we have examples of checked teardown - even if it is for APIs we don't have yet, or for APIs from other platforms - that are not close(2)?

close(2) is peculiar enough (can't be retried; the correct behavior really depends on what the app is doing) that generalizing it to other checked teardown cases seems wrong.

[zackw]

POSIX.1-2008 does leave the state of a file descriptor after a failed close(2) "unspecified", but from the perspective of application code, the only safe thing to do is to assume that the file descriptor has been deallocated, whether or not an error code was returned. In other words, treat it as if the operation always succeeds, but may report an I/O error in the process; that I/O error properly belongs to a previous write operation.

So in terms of trait signatures, I would prefer close to consume self, and in the two-open-files case I would say that it is more important to be sure that both files do get closed, than to reliably report all errors. Lots of programs don't bother checking for delayed I/O errors, and a program that really cares about robustness needs to call fsync anyway.

[zackw]

@federicomenaquintero

Do we have examples of checked teardown that are not close(2)?

Named temporary files might be a good case to think about. unlink can fail for several different reasons, and you certainly do want to report the failure, but I don't think a program would want to retry it, because the failure is probably "sticky" and needs to be addressed by a human. (For instance, someone has messed with the permissions on the parent directory behind the program's back.) Similar to named temporary files are old-style "SysV IPC" resources - shared memory segments, inter-process queues and semaphores - these have their own namespaces, outside the filesystem, and have to be deallocated using special system calls, that again can fail, but for "sticky" reasons.

[Kixunil]

@zackw well said, thank you!

[federicomenaquintero]

@zackw Nice examples, thanks. So, basically:

• Named temporary files - yeah, just report that they couldn't be unlinked. There's nothing the program can do.

• IPC and stuff - I'm thinking of shmat() / shmdt(). The latter can only return EINVAL basically only if you pass it something that was not what shmat() returned. I'd hope that a Rust wrapper for that would make this impossible :) But yeah, I see your point about OS-like calls that can fail on teardown. Just report an error for correctness/completeness, even if the program can't really do anything about it.

[zackw]

The "unlink" equivalent for SysV shm is shmctl(seg, IPC_RMID, 0) which can fail with EPERM. I'm not sure if that's possible for the process that created the segment, but yeah, same treatment as unlink seems best to me.