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

fix: trap with 'Call already trapped' when using futures::stream #450

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mraszyk
Copy link
Contributor

@mraszyk mraszyk commented Dec 9, 2023

This MR fixes the following issue with polling multiple futures using futures::stream: when invoking the method bar of the following canister code

async fn status() {
    let arg = CanisterIdRecord {
        canister_id: ic_cdk::id(),
    };
    let bytes = Encode!(&arg).unwrap();
    let _raw_resp = call_raw(
        Principal::management_canister(),
        "canister_status",
        bytes,
        0,
    )
    .await;
    ic_cdk::trap("trapping");
}

#[ic_cdk::update]
async fn bar() {
    let mut futs = vec![];
    for _ in 0..3 {
        futs.push(status());
    }
    let stream = futures::stream::iter(futs).buffer_unordered(10);
    stream.collect::<Vec<_>>().await;
}

the call fails with "Canister did not reply to the call" instead of the expected "Call already trapped".

This is the root cause I believe. Since the current context's waker (a.k.a. cdk-rs' waker) is replaced by another one, the new waker will be invoked here or here and then the new waker will wake the cdk-rs' waker (see here). Afterwards, it's the cdk-rs' waker responsibility to poll again. However, we don't want to poll again after a trap and thus the cdk-rs' waker will never be woken again after a trap and thus we won't keep trapping for subsequent callbacks. Consequently, the overall update call will fail with with "Canister did not reply to the call" instead of "Call already trapped". Moreover, the state of the outstanding futures will be dropped after the cleanup callback of the trapped future (since we won't poll the parent update call's future) and thus we can trap with "Call already trapped" if we don't find the call future's state in the callback handler which is exactly what this PR implements.

@mraszyk mraszyk requested a review from a team as a code owner December 9, 2023 15:57
@@ -153,6 +153,8 @@ unsafe extern "C" fn callback<T: AsRef<[u8]>>(state_ptr: *const RwLock<CallFutur
// borrow_mut() the state as well. So we need to be careful to not double-borrow_mut.
waker.wake()
}
} else {
crate::trap("Call already trapped");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
crate::trap("Call already trapped");
crate::trap("This call has not been awaited, e.g., because a previous callback trapped");

This could be more precise given an offline discussion with @adamspofford-dfinity

@mraszyk mraszyk marked this pull request as draft December 18, 2023 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant