Am I using `ManuallyDrop` correctly?

code review
1

I have a transaction type (from TiKV for reference) which panics (in debug mode only I think) if you drop it without first calling their rollback() or commit() functions first. Seems simple enough.

I'm creating a wrapping API, and so I have my own transaction type that wraps theirs. I want to make sure I don't accidentally drop my transaction type (and by extension, theirs) so I don't end up with codepaths that unexpectedly lead to panics, even in debug mode.

I tried using std::mem::ManuallyDrop for this, but I seem to have some sort of misunderstanding about its usage.

Here is what I wrote.

/// TiKV transaction.
pub struct TikvTransaction(ManuallyDrop<tikv_client::Transaction>);

impl KvPrimitive for TikvTransaction {
  async fn commit(&mut self) -> KvResult<()> {
    self.0.commit().await?;
    Ok(())
  }

  ...
}

And then somewhere else:

pub(crate) async fn commit<T: KvTransaction>(mut txn: T) -> Result<()> {
  txn.commit().await.context("failed to commit transaction")
}

This second function doesn't interact with ManuallyDrop at all, but is able to move and destruct the transaction without issue, so it feels like there's no "guard" here. I could just as easily write something else that drops the transaction without holding up the invariants.

Am I missing a core piece of regular ManuallyDrop usage? Can it do what I'm trying to make it do? Is there something else for this purpose?

Also is there a more idiomatic Rust API design for dealing with items like this that need to be destructed in specific ways, using the type system?

Anyways, thanks in advance for your input. I love having access to a community like this.

2

You seem to be misunderstanding what ManuallyDrop is used for. It’s not guarding against anything; it doesn’t prevent being destructed itself, instead it’s a wrapper that will just not call the destructor of the contained type implicitly at all. You can easily leak values using ManuallyDrop. Some further API of it involves slightly unsafe functionality to manually call drop on the contained type anyways (this part is unsafe because it cannot guard against use-after-free or multiple calls of the same destructor).

It is not clear what it is that you want then. Rust does not have any support for statically enforcing that a value is not implicitly dropped. (A concept sometimes called "linear types"; in contrast, Rust’s approach is sometimes called "affine types".)

Ah, okay you’re already asking about these points. So indeed, you seem to misunderstand the purpose of ManuallyDrop, and no, the type system can unfortunately not help you here.

The thing the API authors did was already as much as you can do. The best supported thing is usually: Make dropping of an object, at any point, part of its API and have it do sensible things or clean-up actions (another important limitation is that destructors don’t support async).[1]
Or, if reasonably dropping isn’t possible sometimes, then the user must manually make sure the object isn’t dropped when it’s supposed to. The idea of panicking as apparently done in this API to help the user catch bugs that incorrectly drop the transaction. That being said, I’m not sure I fully understand the intended usage pattern of this particular API, so I’m not so sure about the exact consequences of accidentally dropping a Transaction when it isn’t supposed to, anyways.

If your goal is just to remove the possibility for panics, it seems you can use the crate’s API for TransactionOptions to downgrade the panics to warnings or nothing at all. (Switching .begin_transaction_optimistic/pessimistic with some call of .begin_transaction_with_options(TransactionOptions::new_optimistic/pessimistic.drop_check(CheckLevel::…)).)

  1. Unsurprisingly, both points (how to support some version of "linear types" and/or how to support async for destructors) are regularly coming up in discussions for designing future language features of Rust. ↩︎

6 Likes
3

So what I'm understanding is, ManuallyDrop allows you to defer the execution of the type's defined Drop implementation so you can call it yourself. So right now, my usage of ManuallyDrop is just preventing the default panic-on-drop behavior from happening, which is not what I want.

I think I'll just leave the panic-on-drop behavior and do as much as I can to make sure that the transaction is only dropped after commit or rollback.

Really what I wanted was a good way to 1) propagate errors normally and 2) rollback transactions at the same time, because if you just ? an error, it drops the transaction without rolling it back (and if you use a match statement to do both, it adds 5 lines each time, and without any static guarantee).

The solution I generally ended up going with was to use a "consumptive" API, where operations consume the transaction, and if they succeed, they pass the transaction back in a tuple in the return value along with the operation's successful result. If they fail, they rollback and consume the transaction.

It seems odd to me that there's no way to track or disable implicit drops. It seems like that could be a rustc directive, and potentially useful for being really stingy about allocations or something.

4

Yeah, that’s exactly the thing I noticed, too when writing

The API involves a type that you shall not drop implicitly, but also all other functions return Result values, and thus seem to encourage error propagation. This is further shown in example code in the crate documentation itself like

let txn_client = TransactionClient::new(vec!["127.0.0.1:2379"]).await?;
let mut txn = txn_client.begin_optimistic().await?;
txn.put("key".to_owned(), "value".to_owned()).await?;
let value = txn.get("key".to_owned()).await?;
txn.commit().await?;

from the main page. But then again, perhaps the implementation is such that in an Err case, the Transaction is always left in a state that you are allowed to just drop? I don’t know! Really, the documentation does a bad job at explaining the things you must pay attention to w.r.t. dropping Transaction (or I haven’t found the place yet where it is properly documented.)

1 Like
5

Is there some way to make a compiler error if a given function is called? If so, you could implement Drop with that directive inside it to have some form of implicit drop tracking.

6

IDK… maybe in some hacky way? I believe I’ve seen mentions of linker tricks or the like to try to prove statically a lack of panic paths. Even then, the problem is that the compiler can still consider control flow paths that are impossible to happen in practice. Similar to how like a function only called in an if false { … } is usually still considered used, and really only after optimization, such code paths might disappear.

Realistically, the answer is probably just no, there’s no way to do such implicit drop tracking, otherwise some “linear types” feature wouldn’t be so nontrivial, either.

A common idea for statically ensuring drops is callback-style APIs. Instead of constructing a Foo, you just hand out &Foo in a call-back. So fn construct_foo() -> Foo becomes fn construct_foo(impl FnOnce(&Foo)). However, once async is involved, callbacks are also not very neat anymore (as long as proper async closures don’t exist yet). (Also they don’t fully guarantee skipping drops anymore, as the returned Future could be polled a few times, but not completed, and then leaked. But your goal here was just to avoid accidentally forgetting the drop, so that completely hard guarantees for soundness is not really a concern.)

3 Likes
7

Just to note, you may be able to minimize this some by writing something like:

let mut txn = /* ... */;
let op = async {
    /* code using txn and ? */
    Ok(/* ... */)
};
match op.await {
    Ok(ok) => {
        txn.commit().await?;
        ok
    }
    Err(err) => {
        txn.rollback().await?;
        return Err(err);
    }
}

Whether this is doable of course depends on the exact ownership semantics involved.

1 Like
8

You could also write your own Drop implementation which queues a rollback job in the executor if it’s not finalized manually. Something like:

/// TiKV transaction.
pub struct TikvTransaction(Option<tikv_client::Transaction>);

impl Drop for TikvTransaction {
    fn drop(&mut self) {
        if let Some(mut txn) = self.0.take() {
            MyExecutor::spawn(async move {
                while let Err(e) = txn.rollback().await {
                    // log error & implement some kind of backoff
                }
            });
        }
    }
}
1 Like
9

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.