tokio-rs / async-backtrace

MIT License
185 stars 13 forks source link

Can we add frame for a function that reutrns `Poll<T>`? #32

Open Xuanwo opened 1 year ago

Xuanwo commented 1 year ago

Hi, thanks for creating this lib! OpenDAL is working on for adding native integration with this lib to allow users to dump the async backtrace: https://github.com/apache/incubator-opendal/pull/2765.

But we met a problem that we have some trait like AsyncRead returns Poll<T>. Can we add frame for them?

jswrenn commented 1 year ago

Hi @Xuanwo! I apologize for the slow response — I was on an extended leave from work.

I think this is possible. I'll look into it.

jswrenn commented 1 year ago

Can you point me to a type in OpenDAL that implements AsyncRead where you might like to include poll_read in frames?

I think I have a solution that could work for this use-case, but I'd like to be sure.

Xuanwo commented 1 year ago

Can you point me to a type in OpenDAL that implements AsyncRead where you might like to include poll_read in frames?

Sure!

Here is the code where we added async-backtrace: https://github.com/apache/incubator-opendal/blob/main/core/src/layers/async_backtrace.rs.

Currently, we have only added framed for async functions, and we expect to have them on our Readers just like we do for logging: https://github.com/apache/incubator-opendal/blob/main/core/src/layers/logging.rs#L992-L1037.

We plan to return an AsyncBacktraceWrapper<R> and add frame for it's poll_read, poll_seek functions.

jswrenn commented 1 year ago

I'm experimenting with approaches on the more-frames branch: https://github.com/tokio-rs/async-backtrace/tree/more-frames

I've started by making our Frame type public, which makes it possible to crate your own backtrace frames that aren't in the context of a Future, like so (using tokio's Take as an example):

pin_project! {
    #[must_use = "streams do nothing unless you `.await` or poll them"]
    pub struct Take<R> {
        #[pin]
        inner: R,
        // Add a `Frame` field to your type.
        #[pin]
        frame: async_backtrace::Frame,
        limit: u64,
    }
}

impl<R: AsyncRead> AsyncRead for Take<R> {
    fn poll_read(
        self: Pin<&mut Self>,
        cx: &mut Context<'_>,
        buf: &mut ReadBuf<'_>,
    ) -> Poll<Result<(), std::io::Error>> {
        // And run your async routines `in_scope` of `Frame`.
        let this = self.project();
        this.frame.in_scope(|| {
            unimplemented!("method internals go here")  
        })
    }
}

Does this work for you?

This is fine for AsyncRead (which requires that its receiver is pinned), but I see oio::Read doesn't require a pinned receiver. I'm not yet sure how to support non-pinned frames — pinning is fundamental to how this crate is able to keep track of the structure of futures without allocating.

Xuanwo commented 1 year ago

I'm experimenting with approaches on the more-frames branch: https://github.com/tokio-rs/async-backtrace/tree/more-frames

Cool! I will give it a try.

This is fine for AsyncRead (which requires that its receiver is pinned), but I see oio::Read doesn't require a pinned receiver.

oio::Read requires to be Unpin so we don't require a pinned receiver.

Xuanwo commented 6 months ago

Hi @jswrenn, thanks for your work! This solution works for us. However, we've migrated to an async trait design and no longer need this feature.