Add Drop::poll_drop_ready for asynchronous destructors

[withoutboats]

This is the design I worked on last year to allow asynchrony in destructors. It is a relatively small addition, a new provided method on Drop and adjustment to the drop the glue used in async contexts.

Rendered

[Diggsey]

It should also be noted that it introduces silent "yield points" into async contexts, whereas previously all yield points were visible awaits. This is just an extension of the normal behavior of destructors, which already can block, panic, take locks, park the thread, mutate global state, and so on (as they are intended to do).

This is my biggest issue with the concept of async destructors. You mention that destructors can already panic: however, I would consider this bad practice (unless you intend to abort the program) as double-panics cause an abort, and destructors are invoked during unwinding. Personally, I would prefer if all panics inside destructors caused an abort, as it would remove several exception-safety related footguns from the language. Even the other things you mention doing in a destructor (mutating global state, taking locks, etc.) are not indicative of "good code". Sure: there may be times where you need to do those things, but it would be with great reluctance...

The motivations presented in the RFC apply equally well to destructors that are fallible. Currently, you would achieve fallible destruction by having some kind of explicit "close" method which returns a result, and you can do the same thing with async destructors (have an async close method) especially when "drop" needs to work in sync contexts anyway.

I would prefer that Rust moves in a different direction to the one proposed here: one where the role of destructors is reduced rather than expanded. I think that RAII is not the right tool for the job for these more complex cases, and we should instead look towards ideas like linear type systems to solve these cases.

[kennytm]

would there be an async_drop_in_place, or does the user need to manually perform the poll?

unsafe {
    futures_util::future::join_all([
        futures_util::future::poll_fn(|cx| poll_drop_ready(&mut *ptr1, cx)),
        futures_util::future::poll_fn(|cx| poll_drop_ready(&mut *ptr2, cx)),
        futures_util::future::poll_fn(|cx| poll_drop_ready(&mut *ptr3, cx)),
    ]).await;
    drop_in_place(ptr1);
    drop_in_place(ptr2);
    drop_in_place(ptr3);
}

[withoutboats]

I'm not sure what the best approach here is, but possibly this would be helpful. Then we would probably want to change the drop glue description from being as described to instead being described as calling async_drop_in_place in an async context. This would also save a virtual call in the case of trait objects, as they would not need to call poll_drop_ready and then drop_in_place.

Note that for types like vec, for example, the right approach will be to call poll_drop_ready on the slice in Vec's poll_drop_ready method. Slices will have to have drop glue implemented to join a poll_drop_ready on all of their members.

[quininer]

I don't know if it is suitable for discussion here. but vale has some interesting points

It would need to add a !Drop trait, which would signal the compiler that the user can't automatically drop this object, that they instead have to manually call something to drop it.

If we can have this, then we can make drop return a Future without need for a new poll_drop_ready method.

[nagisa]

s/types can which/types which can/?

[nagisa]

Does this need to hold up in presence of panics? Double panics? If one of the asynchronous destructors panics when polled, do we attempt to drive to completion the other destructors? Do we call the non-async drop for the fields which have had their async destructor panic? Do we call non-async portion of the drop glue at all after a panic in an async destructor?

[nagisa]

Consider referencing https://github.com/rust-lang/rfcs/blob/master/text/1857-stabilize-drop-order.md here.

Also consider mentioning how built-in structures like Vec ought to behave and if there are or aren’t any special considerations in presence of panics, especially during structure construction.

[Matthias247]

What is an async context? The body of an async function? An async block? A Future? Anything running in an executor?

[Matthias247]

How is this guaranteed if the async function is in I guess what you describe as a non-async context - which could be a "plain old future combinator"? If there is anything around the async function which isn't aware about the async function, I think it wouldn't be called

[Matthias247]

I wouldn't call this stateless, since fields are clearly stateful - their state is modified by polling. I guess you mean the there is no state to record which fields are already done and that calling the function needs to be idempotent - but this is not what I first think about when reading stateless

[Matthias247]

Unfortunately this approach puts a very heavy burdon on the application developer. They need to be sure that all components they choose for their application are supporting async drop. E.g. in my application I might run a function which intends to perform a database transaction. If my function is cancelled (dropped), I want to rollback the transaction, but need to wait until the rollback completes.

Now I start with a framework which supports async drop. My application works. Then later on the framework gets updated and someone uses a new future combinator. Or I switch out the framework. Then suddenly my remaining code still works, but the rollback will no longer happen - without any code directly related to the rollback having been changed.

That scenario actually horrifies me, and I don't see any other language feature in either rust or any other language which could make that happen.

[withoutboats]

I replied to this point in the out-of-line comment, but to reiterate: this is exactly the behavior of synchronous destructors, which are not guaranteed to run, but are generally relied on by all users. Leaking data without documenting should be considered a serious bug, and beginning to leak data should be considered a breaking change. This is already our community standard around synchronous destructors, and the same process would work fine for asynchronous destructors.

[seanmonstar]

Perhaps something that is unclear is what the default behavior is of some type that contains other values that implemented poll_drop_ready. Is the default that when any type drops in an async context, it will automatically check poll_drop_ready() of all its fields? Or does it only do so if I remember to forward poll_drop_ready on every type that.

Put another way, with code:

struct Envelope<T>(T);

async fn foo() {}

async fn bar() {
    let _e = Envelope(foo());
    // does `Envelope::poll_drop_ready` forward, or did I accidentally "leak"?
}

[withoutboats]

There's a specific section about this; yes, the glue is called automatically, recursively, on fields of types, just like destructors. https://github.com/withoutboats/rfcs/blob/poll-drop-ready/text/0000-poll-drop-ready.md#drop-glue-in-mempoll_drop_ready

This is the same behavior of non-asynchronous destructors.

[seanmonstar]

Oh ok, thanks! I missed that somehow.

[Matthias247]

IMHO one big drawback of this approach is that it doesn't solve the requirement for implementing synchronous drop as a fallback, since there are no guarantees about async drop being called.

That kind of raises the question for application developers on "when to actually implement it", or "why to implement it at all".

[Matthias247]

That's definitely a drawback. But async fn in traits and trait objects is in general a problem that still requires a solution - I'm not sure whether drop is special in that regard.

On the flip side we want people to write asychronous functions and not manual state-machines - because the latter are hard! That's why invented async/await. Inside drop we might want to call functions that are defined as async fn, e.g. for performing the async database rollback, which might lock an async semaphore, and then uses an async socket. Some of those things have pinning requirements, which makes things even harder to get right manually.

[Matthias247]

I think this is a wrong description of it. It doesn't need async destructors because C++ coroutines which resemble async/await tasks run to completion. You can just write your normal function, with a try/finally block if you desire some cleanup.

See my old question to Lewis Baker about this: lewissbaker/cppcoro#102

The same applies e.g. to async/await in Javascript, Kotlin, C#, etc.
The Rust situation is a homemade issue, since it's the only language which represents async functions as futures which are cancellable at any suspension point.

[Matthias247]

Does this now mean we can or can not call async fns inside drop? I think in generally the answer is "no":

• We can not directly await since poll_drop_ready is not an async fn
• The workaround is: Create a Future through the async fn one intends to call, store it somewhere in a pinned location, and poll it on every call from inside poll_drop_ready
• The latter approach doesn't seem to work, since there is no Pin argument

If this is true this would be major limitation, since most of the ecosystem is now moving towards first class support for async functions. And one might e.g. not even be possible to write to an async channel anymore without support for that.

[withoutboats]

We provide specific guarantees about how poll_drop_ready will be called, which one can use to prove the safety of calls Pin::new_unchecked in order to poll those futures.

[Matthias247]

Thanks for taking the time to write this up!

Performing async cleanup work is definitely something that I am currently missing from both an application developer as well as a library developer perspective.

On the application I currently have a project which would in some places simply being broken if someone would cancel Futures at any point in time, since some code might not assume the possibility of not being executed, and synchronous cleanup being hard. That's at the moment tolerable, because the application is small enough to know exactly that nothing would be cancelled - but it's far from an ideal solution. I expect those issues to become even bigger and more prevalent once more developers with a lesser knowledge of Rust and async/await semantics .

On the library side the missing ability to perform async/cleanup work and the ability to defer destruction of tasks prevented so far both nice APIs for completion based IO, as well as scoped tasks with graceful cancellation semantics (tokio-rs/tokio#1879).

Therefore I'm super interested on improving on this.

However I'm afraid the problem I've encountered so far are not really solved by this RFC, and therefore I don't see this improving the situation for projects I've been working on vastly.

The main concern I have is the lack of guarantees. It doesn't seem to be guaranteed that the poll_drop_ready is ever being called. E.g. if I have any Future which implements that method, but which is then wrapped on some higher level with a non async drop aware code (like futures::select!) , it would simply not do anything. Therefore it can't be utilized for things that "need to happen" in order for the application not to become broken - which e.g. could be

• for completion based IO to asynchronously wait until the operation is fully cancelled
• for scoped tasks waiting for the child task to complete

As long as there are no guarantees, it doesn't make sense to use it for those scenarios.
Now obviously there are still some use-cases that could be fulfilled that are outlined here - e.g. flushing a buffer, but those were not the major issues that I encountered so far.

Even if I'm taking memory safety aside, there are some application level requirements which on a first look do not look different from where we are now with this proposal. E.g. if I have the requirement to rollback a transaction on a client disconnect, I would need to perform a call using an async/await database API. As far as I understand this proposal so far, this wouldn't be possible, and I would need to somehow still work around this by delegating the work to a new spawned task.

I would like to get rid of the latter workarounds this these break structured concurrency concepts (which is the nice theoretical term), but practically also have an impact on system reliability and resilience: By doing work outside of the original function/task we lose accounting (e.g. limiting/throttling/load-shedding) for that work and risk being overloaded due to invisible background cleanup tasks. That can again be solved using workarounds, but I would rather prefer a solution for async/await itself which does not make those workarounds necessary.

I had an idea a while about solving the issue by having a separate kind of async function which has the same guarantees about running to completion as regular threads. This has a different set of drawbacks, but so far seems to me the most promising compromise. I might try to dusting off the draft RFC I started a while ago and share it.

[withoutboats]

The main concern I have is the lack of guarantees.

This is already true of course for all destructors. Without this line of code a Rust Vec would not call the destructors of its members when its dropped. And yet, no one is very worried that their data structures do not run destructors properly, because failing to run destructors without clearly communicating that in the documentation is considered a serious, show-stopping bug in the library.

We will need to introduce the same culture shift around poll_drop_ready, so that libraries which implement data structures and certain asynchrony primitives understand their obligations under the new feature set. This will take work on behalf of the project and the community, undoubtedly.

for scoped tasks waiting for the child task to complete

It's interesting you cite this as impossible since its listed as a motivation in the RFC. The pattern of just using an owned guard's destructor to guard scopes is totally unsound, but the sound alternative in which a callback takes a reference to a guard still requires a destructor to be implemented properly, so that the scope will be guarded in the case of panic. If this guard would like to yield the thread and park this task during that period, it needs something analogous to this RFC.

E.g. if I have the requirement to rollback a transaction on a client disconnect, I would need to perform a call using an async/await database API. As far as I understand this proposal so far, this wouldn't be possible, and I would need to somehow still work around this by delegating the work to a new spawned task.

The type implementing drop would need to be able to store the state of the future of that database API somehow. Right now, this may require boxing. The realities of state & their impact on the ergonomics of using this feature are acknowledged in the RFC.

[burdges]

Any chance this helps with issues around destructors in thread local storage ala rust-random/rand#996 rust-random/rand#968 ?

[nagisa]

@burdges from what I can tell not at all. AFAIK accounting for the inherent lack of ordering in TLS destructors can only really be done in library/application specific manner.

[tmiasko]

Would the reference counted pointers have a forwarding implementation of poll_drop_ready to the inner type? Is it possible to provide such implementation?

The proposed drop order makes it challenging if not impossible. Drop::poll_drop_ready on the fields happens before Drop::drop of the struct, so on the one hand, the async drop process of the inner type might have already started, on the other hand, the shared pointer themselves must still remain fully functional.

[withoutboats]

This is a fair question & we need to be careful about shared ownership constructs, which I never investigated very thoroughly. I think there are specifically concerns about cyclic shared ownership constructs which might clone the cyclic Rc in their poll_drop_ready implementation. Do you have specific scenarios in mind we could explore?

[tmiasko]

• The Arc<T> recovers unique ownership over T inside Arc<T>::drop implementation (when it decrements counter to zero).
• Forwarding the Arc<T>::poll_drop_ready to T::poll_drop_ready requires a unique access, but at this point ownership is still shared.

Additionally decrementing the counter in Arc<T>::poll_drop_ready is impossible, because Arc<T> is accessible and must remain valid (according to the proposed drop order).

[withoutboats]

This may be a reason we should return from poll_drop_ready to poll_drop, implemented by default by forwarding to the normal drop.

Rc/Arc would need to be changed to this basic algorithm:

if strong_count > 0:
    decrement strong_count

if strong_count == 0:
    ready!(inner.poll_drop)
    deallocate

This may be possible to implement correctly with poll_drop_ready, too.

Arc/Rc would exist during dropping in a state of having 0 count, but afaict we know we have exclusive access because we decremented the count to 0.

[tmiasko]

Does that mean, that in an asynchronous context, a drop of non-copy temporaries at the end of statement / scope always introduces a yield point? Can it be sometimes avoided, if so on what basis? What about code that drops generic T temporary?

The presence or absence of yield points shapes the overall size of the generator, the determination which lifetimes and types will be included in it, how long the borrows extend, whether resulting future type will by Sync or Send, etc. Introduction of additional yield points to an existing async code base is unlikely to be backward compatible change.

[withoutboats]

Optimizing no-op yield points away is specifically mentioned in the RFC as desirable, and should usually be achievable, maybe benefiting from MIR optimization passes. The biggest concern will be trait objects, because the call will be virtual. We may need specific representational optimizations, such as making the poll_drop_ready pointer null, to prevent that from being a problem.

However, you raise a very good point: this optimization would be necessary to influence the auto trait implementation of async functions, since we must only store state for types which have a meaningful poll_drop_ready in order to avoid a backward compatibility hazard. We probably need to figure out how we could solve that hazard.

It's also an interesting and important point that adding a meaningful poll_drop_ready to a type would be a potentially breaking change, because other non-send types may have to be persisted across that yield point, whereas they weren't before.

Possibly this pushes us to making poll_drop_ready implemented by a separate trait, so that the compiler can more readily track which values have meaningful glue.

[withoutboats]

Actually, it may be that trait objects scupper this plan:

async fn foo(obj: Box<dyn Send>) {
    let rc = Rc::new(0);
    let obj = obj;
}

Can't see a way that this doesn't become a future that no longer implements Send. At very least, implementing this may require crater, lints, and a serious migration effort.

[matu3ba]

destructors are invoked during unwinding. Personally, I would prefer if all panics inside destructors caused an abort, as it would remove several exception-safety related footguns from the language.

@withoutboats
Is unwinding in async context in the official documentation? I did not find anything in the async book.
I found this as open issue about the incomplete documentation (regarding unsafe code). Which one of those is used?
The documentation seems to be still old.

Would an extension to poll_action_ready(), action={Start,Drop,Wait,..} be useful to be mentioned?

Even the other things you mention doing in a destructor (mutating global state, taking locks, etc.) are not indicative of "good code". Sure: there may be times where you need to do those things, but it would be with great reluctance...

The motivations presented in the RFC apply equally well to destructors that are fallible. Currently, you would achieve fallible destruction by having some kind of explicit "close" method which returns a result, and you can do the same thing with async destructors (have an async close method) especially when "drop" needs to work in sync contexts anyway.

I would prefer that Rust moves in a different direction to the one proposed here: one where the role of destructors is reduced rather than expanded. I think that RAII is not the right tool for the job for these more complex cases, and we should instead look towards ideas like linear type systems to solve these cases.

@Diggsey
Can you provide code examples for what you mean? Do you aim for a more general solution or what is the specific critique?

[Diggsey]

Can you provide code examples for what you mean? Do you aim for a more general solution or what is the specific critique?

Sure:

async fn write_to_file() {
    let f = open_the_file().await;
    f.write_all("bleh").await;
    f.close().await;
}

A linear type system would mean that failing to call f.close() would result in a compiler error, since f was dropped and this is not allowed.

If this function was fallible, it would also fail to compile:

async fn write_to_file() -> Result<(), Error> {
    let f = open_the_file().await?;
    f.write_all("bleh").await?; // Error: `f` is implicitly dropped
    f.close().await;
}

Using today's syntax, you'd need to explicitly handle that, ensuring that f is never implicitly dropped:

async fn write_to_file() -> Result<(), Error> {
    let f = open_the_file().await?;
    let res = f.write_all("bleh").await;
    f.close().await;
    res
}

If this becomes too verbose, we could look to improve the syntax (for example, the try/finally blocks from a previous RFC would make it easier to clean up correctly in more advanced cases). The great advantage is that any ergonomic improvement here will also help with destructors that are fallible: there's nothing stopping close() from returning an error now.

This could be introduced using a new auto marker trait (ImplicitDrop or something) such that types not implementing the trait would have to be deconstructed instead of dropped (or passed to a function that in turn deconstructs them).

This RFC doesn't touch much on unwinding, other than to state that the RFC would allow asynchronous cleanup whilst unwinding is occurring. This seems complicated and dangerous to me, as it would allow a panic to be "in-flight" on a suspended task! Some questions about that:

• What if the task itself is dropped whilst a panic was in-flight already. Does the panic just get ignored?
• What if other tasks panic too, and you end up with large numbers of in-flight panics all waiting on some async operation that will never finish?
• What should thread::is_panicking return whilst the panic is suspended?
• How does an in-flight panic get stored in a task?
• Do all tasks need to be UnwindSafe in order to poll them? (Since without that bound, you can observe the state of a suspended task while a panic is in-flight)

In my proposal, in the case of unwinding due to a panic, f would be required to clean up after itself synchronously, or abort the program if that's not possible. Asynchronous unwinding is just not something I want to get into...

[Aaron1011]

Is this a freestanding function which takes &mut self?

[bestouff]

Would it be possible to explicitly add that poll_drop_ready may not be called, but when it's called it's in a loop at least until it returns Poll::Ready(()) ?
(Yes it's been already said above, but I feel it should also be in the "guarantees" section)

[withoutboats]

A linear type system would mean that failing to call f.close() would result in a compiler error, since f was dropped and this is not allowed.

Linear types do not play well with generic parameters. Consider your linear file type, what happens when I pass it to mem::drop, or any other function that takes any type T as a parameter by ownership? It's dropped at the end of those functions, without calling their "explicit destructor." Clearly, to implement this backward compatibly we'd need some new ?Trait, and these types could not be passed as type parameters to anything which hasn't been modified to add ?Trait. This would represent an enormous, frustrating, and ugly ecosystem-wide migration.

Even if it weren't a huge backwards compatibility hazard, in order for types to return different finalized outcomes (ie different results or futures), they'd all need different associated types, which a generic function would not be able to handle. For drop, this is simple enough:

// all type parameters are implicitly bound `Nonlinear`, just as they are implicitly bound `Sized`
trait Nonlinear { }

// types which explicitly implement Linear do not implement Nonlinear
trait Linear {
    type Outcome;
    fn finalize(&mut self) -> Self::Outcome;
}

// new alternative to drop which must be explicitly called on types that dont implement Nonlinear
fn finalize<T: ?Nonlinear>(arg: T) -> T::Outcome { /* intrinsic */ }

But obviously for more complex functions which take multiple generic parameters by value, they now have to return a tuple of their real return value and the outcome of the finalizers of all of their generic parameters, so that their callers can process them.

Because of these issues, I see little chance of this progressing further in Rust in any near or medium term time horizon. The limitations on drop that make it potentially error prone exist because they are necessary to make it tractable.

---

This RFC doesn't touch much on unwinding

Agreed, the behavior of unwinding needs to be thought through more carefully.

[Diggsey]

Clearly, to implement this backward compatibly we'd need some new ?Trait, and these types could not be passed as type parameters to anything which hasn't been modified to add ?Trait. This would represent an enormous, frustrating, and ugly ecosystem-wide migration.

Yep, but this is no different than many other features we add (const fn, nostd, etc.) and migration can be done on-demand as need for it becomes apparant. The reality is that most existing code is not going to work with any form of async drop, because existing code expects to be able to drop types synchronously: this RFC side-steps the issue by providing a "default behaviour" when there's no obvious way for async drop to be called, but that default beheviour will be wrong, or at least not ideal in many cases, and so will still result in an ecosystem-wide migration to "support" async destructors properly in many libraries. What's worse to me is that there's no way of knowing, as a user, whether a library does "do the right thing" (whatever that means for the particular library in question) because the code will compile fine either way.

Even if it weren't a huge backwards compatibility hazard, in order for types to return different finalized outcomes (ie different results or futures), they'd all need different associated types, which a generic function would not be able to handle. For drop, this is simple enough:

This is not what I described: there's no need for a "finalize" trait, or to modify the bounds on "drop" because linear types cannot be dropped (except through unwinding). f.close() is just a normal method that can return whatever it wants. The only thing a linear type system does is throw up a compile error if a linear type would be dropped on any execution path. The implementation of f.close() would consume self and destructure it into its constituent parts, thereby avoiding the type being dropped at any point.

Agreed, the behavior of unwinding needs to be thought through more carefully.

IMO, this is quite an important aspect of the RFC to omit, and "scope guards" should not be listed as a motivation for this solution unless that is addressed.

[withoutboats]

gonna close this, now the file is uploaded and the async WG could take this over after streams if they want. i dont want to deal with this rfc thread. im not getting paid to do this and it makes me very unhappy

[Ekleog]

Not sure where this discussion should move to, so I'll continue here, hoping boats unsubscribed to not make them feel worse. If the conversation moves elsewhere, please just poke here so people interested in this RFC can know where it's moving towards?

FWIW, I don't see what backwards compatibility hazard would be brought by a NonLinear auto trait like Sized: all code that currently works will continue working, and only new code could stop working.

That said, indeed it's something that'd probably be not-very-well-supported through the ecosystem (like !Sized types are), but the parts of the ecosystem that actually do need to support it probably will support it soon enough: relaxing an unneeded bound should be pretty easy, and it'd maybe even be possible to automatically have : ?NonLinear for functions that take the type only by reference (though it definitely needs more thought due to things like Clone). Actually, I'd be pretty sure it'd quite fast be better-supported than !Sized types are, because it's almost a mechanical refactor, while we don't even have alloca for !Sized types.

On the other hand, async destructors would certainly involve quite a few corner cases (at the very least around unwinding and cancelling the task mid-drop), that I feel would have a worse impact on the overall ecosystem (by creating bugs in previously correct code) than a forced addition of : ?NonLinear on the few functions that look like T -> T.

Side-note: if going the linear type way, we may want to add a AsyncDrop trait to emulate this RFC, so people could take : ?NonLinear + AsyncDrop if they do need to take T and drop it but are OK with it being dropped asynchronously — and could also add FallibleDrop and the like: they'd all be traits we could develop in crates and see whether they're actually being needed (my guess would be no)