Private drop, or "Rust could be better at RAII with a rather small change"

#1

I often manipulate object that have some release logic.
To illustrate the flaw in the current situation, I’ll take the example of BufWriter.

Of course, nobody would ever want to write in a BufWriter without flushing the result in the end.
Quite naturally, BufWriter calls flush on Drop (See BufferedWriter#Drop).

There are two problems to use Drop to enforce release logic.

a) What if release can return an error. In the case of the BufWriter, the call to flush() may return an io Error that will be ignored by the BufWriter’s drop, and give us a false sense of safety : Any code that relies on Drop to flush its BufWriter is basically faulty

b) The call to Drop is implicit. Putting complex logic in it, especially if it has performance impact, panics, or side-effect is a bad idea. Unfortunately release logic may be complex.

A simple solution could be to somehow able to declare the Drop implementation as “private”. The effect would be that the object cannot be dropped from the scope outside of one of its method.

private impl Drop for BufWriter {}

After this addition, the compiler will not let any BufWriter to “leave the scope”.
To get his code to compile, the user would need to call a release function.

The name is up to the library writer, and there could be more than one for complex cases.
The idea is that it would use move semantics for self. For instance, in the case of the BufWriter,

fn close(self) -> io::Result<()> {
   if self.inner.is_some() && !self.panicked {
            self.flush_buf()
   }
   else {
            Ok(())
   }
}

would make sense.

What do you think?

#2

Here’s some discussion of the similar feature: https://gankro.github.io/blah/linear-rust/.

The effect would be that the object cannot be dropped from the scope outside of one of its method.

Note that this won’t work as is, because panics call drops. So, BufWriter still need to have logic for “looks like something crashed badly, let’s try to optimistically flush buffers”.

I think a reasonable practical solution would be to add debug_assert(self.flushed || ::std::thread::panicking(), "Please, call BufWriter::flush explicitly"), coupled with a lint.

#3

The trouble is how would this work in other contexts where drop is called implicitly? Say storing a new value into something that has Drop, or even into a slice/Vec of such things. It might get slightly unwieldy/noisy in those cases.

I like @matklad’s practical compromise. As a design pattern, offer up fallible alternatives that users can call explicitly.

#4

Since Drop may still be necessary for emergency cleanup, why not have an ExplicitDrop (or some other name) marker trait that requires the value to be consumed before it leaves the scope? There would have to be some kind of exception to the rule somewhere, though, otherwise it turns into some kind of hot potato. Maybe it can only be destructured? Maybe it’s ok to drop within one of its methods?

#5

@matklad Thanks for pointing out panic!. That’s an excellent point. I’m not fond of the dynamic check, well, as it introduce a dynamic panick on Drop, but I agree it would be better than the current situation.

I actually use this exact trick in tantivy’s RamDirectory.

@ogeon Yes, you are correct. A marker would probably a smarter way to do that.

#6

I’ve caught myself implementing a similar drop pattern an nth time today, so this time I am publishing this as a micro-crate: https://github.com/matklad/drop_bomb :slight_smile:

#7

In this example from the docs for drop_bomb:

fn close(mut self) -> Result<()> {
    self.bomb.defuse();
    self.inner.flush()?;
    Ok(())
}

it seems like defuse is called unconditionally – whether or not there is an error from flush. Shouldn’t it be:

fn close(mut self) -> Result<()> {
    self.inner.flush()?;
    self.bomb.defuse();
    Ok(())
}

or am i misunderstanding how the library works?

#8

Yes, the bomb should indeed be defused unconditionally in the .close call. .close consumes self and acts as a destructor. The doomsday machine bomb ensures that the user of CheckedBufWriter uses it correctly.

In particular, it prevents this problematic code:

fn foo() -> Result<(), io::Error> {
    let writer: BufWriter = create_buf_writer();
    writer.write(data)?; // checking for errors, good.

   // oups, the writer is dropped here, and the potential error on flush is swallowed
}

with CheckedBufWriter, you’ll get a panic, unless you call .close() explicitly, in which case you’ll be forced to handle the error.

The example in the docs (and docs in general) could certainly be improved, but I am busy with other stuff atm, so I’ve decided to publish the crate without 100% of polish :slight_smile: Pull-requests, and especially documentation improvements are always welcome!

#9

In the example from the docs, an error in flush would be returned to the calling function. In your modified version, an error in flush would drop self without defusing the bomb, causing a panic.

#10

No, the docs and example were fine – I just got mixed up somehow!

#11

BTW, errors in fn close(self) functions a pain, because if an error happens, the handle is already gone, and there’s no way to retry!

I’ve ran into a similar problem in rusqlite which could fail to commit, but because fn commit(self) was eating the transaction, it didn’t give a chance to retry or roll back the still-open uncommitable transaction.

#12

That’s a good point to raise. One pattern I’ve seen is to make it so that the error type returned by a self method contains a way to get the original self object back so that you can try again.