Is the `StaticExecutor` not updating the `Task`'s reference count correctly?

[songzhi]

This is a reproduction:

// async-task v4.4.0
impl RawTask {
  unsafe fn destory(ptr: _) {
    // ...
    // Finally, deallocate the memory reserved by the task.
    alloc::alloc::dealloc(ptr as *mut u8, task_layout.layout);
    std::eprintln!("destroyed, ptr: {:p}", ptr);
  }
}

// async-executor
#[cfg(test)]
mod tests {
    use super::*;
    use std::boxed::Box;

    #[test]
    fn task_refs() {
        let ex: &'static StaticExecutor = Box::leak(Box::new(StaticExecutor::new()));
        // let ex = Executor::new();
        {
            ex.spawn(async move {
                futures_lite::future::pending::<()>().await;
                unreachable!();
            })
            .detach();
        }

        for _ in 0..100 {
            ex.try_tick();
        }
        std::mem::forget(ex);
    }
}

Run test task_refs, we can see the destroyed, ptr: .. printed, if we use Executor， it will not print. I guess this is a bug?
The reference count of task is 1, and in the last of RawTask::run, it go through this line: https://github.com/smol-rs/async-task/blob/a11c4b22cbcbbdbc4fa0ff62bdbffb430a9d3394/src/raw.rs#L664 , so the task's memory was dealloced.

Maybe relevant PR: #112

[songzhi]

    unsafe fn spawn_inner<T: 'a>(
        &self,
        future: impl Future<Output = T> + 'a,
        active: &mut Slab<Waker>,
    ) -> Task<T> {
        // Remove the task from the set of active tasks when the future finishes.
        let entry = active.vacant_entry();
        let index = entry.key();
        let state = self.state_as_arc();
        let future = AsyncCallOnDrop::new(future, move || drop(state.active().try_remove(index)));

        let (runnable, task) = Builder::new()
            .propagate_panic(true)
            .spawn_unchecked(|()| future, self.schedule());
        entry.insert(runnable.waker());

        runnable.schedule();
        task
    }

Maybe we should put this logic about reference counting into async-task's Task detaching? For example, detach a Task add a extra reference, and decrease it in extra when the future is completed.

[notgull]

Nice catch! I think this problem can be solved by having the task keep track of its own handle.

[notgull]

Actually, is this a bug in async-task? I'm wondering if it's closer to intended behavior.

async_task::Task is kept alive as long as there is a Waker referencing the task. Since pending() doesn't keep the Waker alive, it realizes that the Task is not kept alive and just drops it.

The main issue is that this code would be surprising:

struct PanicOnDrop;
impl Drop for PanicOnDrop {
    fn drop(&mut self) {
        panic!();
    }
}

let ex = StaticExecutor::new();
ex.spawn(async {
    let x = PanicOnDrop;
    std::future::pending::<!>().await
}).detach();
for _ in 0..100 {
    ex.try_tick();
}

Since it would panic instead of completing successfully, since the task is dropped. However, it is correctly detecting that the task will never be queued again.

We should probably discuss more on this. My gut instinct here is that this is intended, since this kind of edge case is unlikely in production code and it would require additional overhead to prevent it from happening.

cc @james7132

[james7132]

StaticExecutor is designed with the idea that it would never be dropped and is 'static in every sense, so a task that never wakes would permanently leak until the end of the program if detached.

In such a case, this does seem to be working as intended, unless there is a leak when detaching Tasks that could otherwise be driven to completion.

[notgull]

@james7132 I think you missed the issue. The issue is not that the task is being leaked; this is the expected result. The issue is that the task is being dropped prematurely. I'd argue since that there are no live wakers that this is intended.

[james7132]

From the documentation on std::task::Waker:

The typical life of a Waker is that it is constructed by an executor, wrapped in a Context, then passed to Future::poll(). Then, if the future chooses to return Poll::Pending, it must also store the waker somehow and call Waker::wake() when the future should be polled again.

Pending's current implementation does not strictly adhere to the storage requirement. While the observed behavior is surprising, I'm of the opinion that its a result of not upholding that part of that contract., and thus is the responsibility of Pending (or any other T: Future) here. I don't think this is intended behavior given that this isn't documented anywhere on Pending's docs, but I don't think it's the fault of StaticExecutor's or async_task implementation here.

[notgull]

I agree. Maybe it's an issue to raise with the Rust design team.