Lint for types where cleanup before drop is desirable

Opened by Stepan Koltsov at 2019-09-01 14:15:43

BufWriter.drop is broken.

BufWriter flushes data on drop and ignores the result. It is incorrect for two reasons:

you must not ignore write errors. For example, when filesystem is full, write fails. Currently last write error is quietly ignored. This code demonstrates the problem: http://is.gd/ZdgbF9
drop may indefinitly hang, for example, if BufWrtiter underlying stream is socket, and nobody reads on the other side

Generally, I think any drop must only release resources, do not do anything blocking or failing.

The similar issue was only partially fixed in #30888.

We (together with @cfallin) propose a solution:

Proposed solution

Add another function to BufWriter (and probably to Write trait): cancel. User of BufWriter must explicitly call either flush or cancel prior to drop.

struct BufWriter gets unfinished flag instead of panicked.

BufWriter.write unconditionally sets unfinished = true.

BufWriter.flush or BufWriter.cancel unconditionally set unfinished = false.

And finally, BufWriter.drop becomes an assertion:

impl Drop for BufWriter {
    fn drop(&mut self) {
        // check `flush` or `cancel` is explicitly called
        // but it is OK to discard buffer on panic
        assert!(!self.unfinished || std::thread::panicking());
    }
}

That change is backward incompatible, however, it is not that bad: developer will likely get panic on first program run, and can quickly fix the code.

Change could be transitional: no-op cancel function could be added in rust version current+1 and assertion added in rust version current+2.

Seems to be related to #32255, i.e the same concern of closing (or cancelling) & handling errors before drop.

Due to the terrible events that unfold if you panic in unwinding (= abort), panic in a destructor implementation is not a good idea.
bluss at 2016-04-02 10:44:26
Interesting -- if no panic, then there are two possibilities on drop when unwritten bytes remain:
1. Flush to the underlying stream, swallowing errors silently, or
2. Drop the unwritten bytes silently.
The discussion came up on rust-protobuf because its output stream abstraction (CodedOutputStream) takes option 2 above, and we compared its behavior to BufWriter. Option 1 seems less surprising to me, but I can see the arguments for option 2, especially the ``stop everything quickly and don't hang on IO'' use-case. Our either-flush-or-cancel-explicitly idea was an attempt at reconciling the two.

Perhaps a good solution here is just to add a cancel() or cancel_unflushed_bytes() method on BufWriter that drops any unwritten bytes, to give the user that flexibility? The semantics are a little weird (the number of bytes dropped is implementation-dependent) so I could see an argument for making it unsafe too (though I'm not sure if that's appropriate, as it seems unsafe is usually for potential memory- or race-unsafety). What do you think?
Chris Fallin at 2016-04-02 13:39:43
I expect the libs team will be helping us in this discussion.

Some I/O resources are not well modelled with RAII at the moment, and should rather be linear types == you have to call a consuming operation on them (maybe in rust it does not need to be consuming, just disabling).

File, BufWriter, etc, need to first have “eager” error handling available to call, such as .close(), .flush(), or .cancel().

To not have any breaking changes, instead introduce lints that sternly guide the user towards ensuring the final methods are called on linear resources before they go out of scope. To make this complete, maybe the lint must even suggest that something that contains a File or BufWriter is also adding a final method.

Ensuring this up front is more in line with Rust's philosophies than using drop and panic.
bluss at 2016-04-02 14:07:36
See also https://github.com/rust-lang/rfcs/pull/1568.

Generally, I think any drop must only release resources, do not do anything blocking or failing.

I do not see a problem here personally. One can always manually run the flush and the standard library prevents you from shooting self in the leg if it is not called.

I would rather it not ignore the errors which happen on flush and panic, but maybe there’s a reason for that (double-panics is not a good-enough one).

developer will likely get panic on first program run, and can quickly fix the code.

That’s a no-go breakage for non-major release in my eyes.
Simonas Kazlauskas at 2016-04-02 21:54:09
I think the motivation behind this issue sounds good to me, but I think that the conclusions it reaches are untenable unfortunately. Rust is generally pretty good about helping you not ignore errors, and the buffered writer does indeed make it easy to ignore the error of the last write.

In cases like this we need to be sure to add some method of seeing the error, and as pointed out by @nagisa in this case it's the flush method. If you're diligent to call that, then an error can never happen in the Drop implementation.

Some thoughts on the proposal:
- Errors being ignored in Drop was a policy decision made long ago. This prevents double panicking leading to an abort.
- Rust doesn't have the ability to statically guarantee that a method is called on a type (e.g. it's explicitly dropped), so enforcing this at runtime would be such an ergonomic hit I don't think it'd be tenable.
- This all seems very similar to the "what do we do if close returns an error problem", and I think the solution is basically the same (just give a method that gives you the error, and you can call it if you're diligent).
We can perhaps investigate lints or some other method of helping programs call flush or whatever the method is, but I don't think we want to change the semantics of the default BufWriter
Alex Crichton at 2016-04-03 00:33:59
We could add something similar to the must_use attribute + lint. Let's call it do_not_drop. adding the do_not_drop attribute to a type, means it gets linted against if you don't move out of it explicitly. Then we can add a cancel and a new flush method to BufWriter that consumes the BufWriter (and thus doesn't call Drop::drop) and returns a Result.

This way there's no breaking change, but everyone starts getting a warning + an easy fix.
Oli Scherer at 2016-04-05 07:22:03
@alexcrichton

In cases like this we need to be sure to add some method of seeing the error, and as pointed out by @nagisa in this case it's the flush method.

In sys::unix::fs, flush does nothing and returns Ok.

In POSIX, the only portable way to observe a final write error is by observing the result of close. There's nuance with EINTR, but I'd argue that in this case it's better to return an Interrupted error and flag the file object as closed (retaining into_raw_fd access on OSes where the file descriptor stays valid in this case).
Mikhail Zabaluev at 2016-04-07 10:50:22
The flush that was referred to was BufWriter's.

Using close's return status as a source of truth that all of the writes succeeded is a somewhat dangerous notion. If you want to ensure data integrity, you'd really need to fsync the data, as the man pages for close notes:

A successful close does not guarantee that the data has been successfully saved to disk, as the kernel defers writes. It is not common for a file system to flush the buffers when the stream is closed. If you need to be sure that the data is physically stored use fsync(2). (It will depend on the disk hardware at this point.)

This is exposed as the sync_data method on File.
Steven Fackler at 2016-04-07 15:20:45
How about offering the end user maximum flexibility? The writer types could have a "DropPolicy" parameter (in the sense of c++ policy-based design), with maybe some default, but still allowing the end user to choose behaviour.
elferdo at 2016-12-30 18:09:07
@oli-obk an alternative might be to mark types as "require explicit drop" and allow those types to provide a drop implementation with a custom signature.

That is, a user would then be required to explicitly call drop on the object, drop would then consume the object and could return an enum that its either empty (success), or contains a (self, error_code) with the consumed object and the error, such that the user can retry the drop operation.
gnzlbg at 2017-01-16 13:50:44
cc https://github.com/rust-lang/rfcs/pull/2677
Alex Kladov at 2019-04-11 07:45:31