Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async Drop #3541

Closed
wants to merge 5 commits into from
Closed

Async Drop #3541

wants to merge 5 commits into from

Conversation

wiseaidev
Copy link

@wiseaidev wiseaidev commented Dec 12, 2023

Introduction

This PR proposes the introduction of native async drop support in Rust by adding the async fn drop method to the Drop trait. Additionally, it suggests the creation of an AsyncDrop trait, allowing us to define custom asynchronous cleanup logic.

Motivation

The primary motivation is to standardize asynchronous resource cleanup in Rust, streamlining adoption and reducing dependency management overhead. This enhancement addresses the current gap in Rust's support for async lifetimes, making the language more versatile for modern asynchronous applications.

Implementation and Benefits

The design introduces native async drop support through the async fn drop method, providing a standardized way to handle asynchronous cleanup. The proposal also outlines an AsyncDrop trait, enabling us to define custom asynchronous cleanup logic for our types. This native support simplifies codebases, reduces external dependencies, and fosters a more consistent and streamlined experience for us working with asynchronous resource management.

Rendered

@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Dec 12, 2023
@python-ast-person
Copy link

python-ast-person commented Dec 14, 2023

How would drop glue be handled?

@wiseaidev
Copy link
Author

How would drop glue be handled?

Good question! In my initial thought, I considered adopting a LIFO approach, much like the synchronous case. This is evident in the management of let bindings, as illustrated in the following code snippet:

#[tokio::main]
async fn main() {
    {
        let async_res1 = AsyncResource {
            data: String::from("Example Data 1"),
        };
        let async_res2 = AsyncResource {
            data: String::from("Example Data 2"),
        };
        // The drop method will be triggered here
    }
    // `async_res2` will be dropped first, followed by `async_res1`
}

In this case, I anticipated that the drop method would be invoked within the inner scope, leading to the destruction of async_res2 first, followed by the destruction of async_res1 due to the LIFO nature of the stack.

However, upon reconsideration, I am now pondering the implications within the context of the event loop. The event loop introduces an asynchronous dimension, raising questions about how these asynchronous resources will be dropped.

For this reason, I am open to any further refinements of the RFC.

@clarfonthey
Copy link
Contributor

Is this not just proposing to add garbage collection back into the language?

I mean, what you're effectively proposing is to, instead of immediately dropping resources when they fall out of scope, to mark them for reclamation and add them to a task queue. That's basically what asynchronous drop is, after all. Except this is obviously a much more naïve implementation.

The main concern I have with this approach is that it can be very confusing to now have this weird matrix of drop traits depending on what is implemented. I would expect there to be some safeguard for dropping things in a sync context with only dedicated AsyncDrop code, and similarly for those with only a Drop implementation. You don't run into unsafety, but you do end up with confused users IMHO.

There is value to complicated destructors, I just personally avoid them and thus can't see much value in this implementation since it seems prone to lots of error. If you want to do anything with blocking I/O in a destructor, that feels wrong to me, so, what other cases would you want to block?

@wiseaidev
Copy link
Author

Is this not just proposing to add garbage collection back into the language?

No. It is more like improving the ownership model. It is like instead of throwing out things when you're done with them, carefully execute some custom logic when dropping these items. It's not about reintroducing garbage collection; it's about upgrading our ownership model to be more refined and efficient.

The main concern I have with this approach is that it can be very confusing to now have this weird matrix of drop traits depending on what is implemented. I would expect there to be some safeguard for dropping things in a sync context with only dedicated AsyncDrop code, and similarly for those with only a Drop implementation. You don't run into unsafety, but you do end up with confused users IMHO.

The introduction of AsyncDrop serves as a specialized mechanism for items necessitating special asynchronous handling right at the moment when the drop kicks in. This procedural differentiation is like categorizing items for systematic organization. More like performing a custom logic and special asynchronous cleanup when the item is dropped.

There is value to complicated destructors, I just personally avoid them and thus can't see much value in this implementation since it seems prone to lots of error. If you want to do anything with blocking I/O in a destructor, that feels wrong to me, so, what other cases would you want to block?

Yes. I am currently researching this area. I think AsyncDrop can be particularly useful in scenarios where cleanup or finalization operations associated with a resource involve asynchronous tasks like the following scenarios:

  1. Async I/O Operations: Let's say we have an object representing an open network connection, and the cleanup process involves closing the connection, which is an asynchronous operation. AsyncDrop allows you to integrate the asynchronous cleanup logic easily.
trait AsyncDrop {
    async fn drop(self);
}

impl AsyncDrop for AsyncConnection {
    async fn drop(self) {
        // Async operation to close the connection
        self.close().await;
    }
}
  1. Database Connections: When managing resources like database connections, releasing the connection back to the pool or ensuring proper closure may involve asynchronous actions. Async drop can be beneficial in such scenarios.
trait AsyncDrop {
    async fn drop(self);
}

impl AsyncDrop for DatabaseConnection {
    async fn drop(self) {
        // Async operation to release the connection to the pool
        self.release_to_pool().await;
    }
}

and so forth. In these examples, async drop allows you to encapsulate and execute asynchronous cleanup logic when an object goes out of scope, providing a convenient and structured way to handle resources that involve asynchronous operations during cleanup.

Finally, I am open to additional enhancements or further refinements to this RFC.

@SkiFire13
Copy link

    // AsyncResource is dropped as it goes out of scope, and its asynchronous drop logic may be executed later by the Tokio runtime

AsyncResource may have been pinned so it cannot be moved from the current scope, meaning the current scope must be active until it is actually dropped. So it cannot be dropped "later". Not to mention this is missing any detail on how this can even reach the executor. Consider for example the simpliest executor:

use std::future::Future;

fn block_on<T>(fut: impl Future<Output = T>) -> T {
    use std::pin::pin;
    use std::task::{Waker, Context, Poll};

    let waker = Waker::noop();
    let mut cx = Context::from_waker(&waker);
    let mut fut = pin!(fut);
    
    loop {
        if let Poll::Ready(value) = fut.as_mut().poll(&mut cx) {
            return value;
        }
    }
}

if fut contained some variable with an async drop what would happen, and when would the async drop be executed?

The implementation ensures that the asynchronous drop logic is executed in the appropriate context, facilitating proper resource cleanup in async scenarios.

What is the "appropriate context" here?

@python-ast-person
Copy link

How would drop glue be handled?

Good question!...

I don't think I communicated my intent properly. What I meant is for code like this:

struct MyStruct{
    a:AsyncResourceA,
    b:AsyncResourceB,
}

async fn my_fn()->usize{
    let mut example = MyStruct::new();
    return example.do_stuff();
}

how does the compiler handle the dropping of each of the fields of MyStruct?

@wiseaidev
Copy link
Author

Thanks for your contribution to the conversation, @SkiFire13! I have updated the RFC accordingly.

text/3541-async-drop.md Outdated Show resolved Hide resolved
text/3541-async-drop.md Outdated Show resolved Hide resolved
text/3541-async-drop.md Outdated Show resolved Hide resolved
text/3541-async-drop.md Outdated Show resolved Hide resolved
@ds84182
Copy link

ds84182 commented Dec 16, 2023

This is written using the same method from the author's previous RFC.

There are many parts of the RFC that reek of this gunk. It contains long, drawn out sentences ("High fructose corn sentences") and plenty of internal inconsistencies.

I understand everyone is trying to respond and critique in good faith despite the author's past actions, but I would rather this time be spent on an actual human written RFC with meaningful content.

@wiseaidev
Copy link
Author

This is written using the same method...

I don't want to address this statement because it is counterproductive. If you have additional points to share, please do so. We all make mistakes, and I'm far from perfect. While I have the option to submit an RFC from an alt account, I'm not afraid or hesitant to reveal my true identity, even if it could have consequences for my career.

There are many parts of the RFC that reek of this gunk. It contains long, drawn out sentences ("High fructose corn sentences") and plenty of internal inconsistencies.

We would greatly appreciate your help in enhancing this RFC. Your insights and suggestions are invaluable in refining and optimizing the proposal.

I understand everyone is trying to respond and critique in good faith despite the author's past actions, but I would rather this time be spent on an actual human written RFC with meaningful content.

Again, we all use LLMs in one way or another. Saying otherwise is a straight-up lie.

@wiseaidev
Copy link
Author

Thanks for the follow-up @python-ast-person. I am going to address your concerns.

Copy link

@SkiFire13 SkiFire13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be my last review here. It is clear who or what wrote this RFC lacks understanding of basic Rust features, not to mention async. You should focus on that before proposing changes to the language.

I spent way too much time reading this proposal and I'll avoid further comments.

Again, we all use LLMs in one way or another. Saying otherwise is a straight-up lie.

Speak for yourself, I've never used a LLM except for testing what they are capable when they first became available. I recognize they can be useful to setup a paragraph but whatever technical information they split out cannot be relied upon.

text/3541-async-drop.md Outdated Show resolved Hide resolved
text/3541-async-drop.md Outdated Show resolved Hide resolved
@wiseaidev
Copy link
Author

Once again, thank you to everyone who has contributed to this conversation to improve the RFC. I will dedicate further efforts to its enhancement over the weekend. Your valuable contributions are greatly appreciated.

@wiseaidev wiseaidev marked this pull request as draft December 16, 2023 19:12
@technetos technetos closed this Dec 18, 2023
@technetos
Copy link
Member

AI generated RFCs are not allowed here.

@rust-lang rust-lang locked and limited conversation to collaborators Dec 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants