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 <https://github.com/brson/rust-api-guidelines/issues/61#issuecomment-314423930>, 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.

[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.