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: lazy CallFuture #391

Merged
merged 11 commits into from May 12, 2023
Merged

fix: lazy CallFuture #391

merged 11 commits into from May 12, 2023

Conversation

mraszyk
Copy link
Contributor

@mraszyk mraszyk commented Apr 26, 2023

Description

This PR updates the semantics of CallFuture to only make the inter-canister call (calling ic0::call_new, ..., ic0::call_perform) when the future is actually awaited. In particular, if the future is never awaited, then the inter-canister call is not performed at all. Otherwise, firing the call is postponed to the code location that awaits the future.

Motivation

Given the current semantics of CallFuture, it is possible that the inter-canister call is fired from a call context C that creates the future while the future is only awaited in another call context D. Depending on the actual message scheduling, this leads to one of the following scenarios:

  • the future has already finished execution when it is awaited in the call context D: then the continuation after the await is executed in the call context D (as expected);
  • the future has not yet finished execution when it is awaited in the call context D: then the execution of call context D is suspended (and potentially starved if D has no outstanding calls at this point) and the continuation after the await point is executed in the call context C (which is unexpected).

We can exemplify the unexpected case with the code snippet below (thanks to @crusso ):

thread_local! {
    pub static CELL: RefCell<Vec<Pin<Box<dyn Future<Output = Result<(u32,), (RejectionCode, String)>> + Send>>>> = RefCell::new(vec![]);
}

#[update]
async fn task() -> u32 {
  ic_cdk::api::print("task");
  return 888;
}

#[update]
async fn writer() -> u32 {
  print("writer");
  CELL.with(|v| {v.borrow_mut().push(Box::pin(call::<(),(u32,)>(id(),"task",())))});
  call::<(),(u32,)>(id(),"task",()).await;
  42
}

#[update]
async fn reader() ->  u32  {
  print("reader");
  let o = CELL.with(|v| { v.borrow_mut().pop() });
  match o {
      Some(f) => {
          f.await.unwrap();
          print("returning 666 from reader?");
          return 666;
      }
      None => {
          call::<(),(u32,)>(id(),"reader",()).await;
          return 1;
      }
  }
}

#[update]
async fn go() {
    let y = call::<(),(u32,)>(id(),"writer",());
    let x = call::<(),(u32,)>(id(),"reader",()).await;
    let w = y.await;
    print(format!("reader: {:?}", x));
    print(format!("writer: {:?}", w));
}

Here a future stored in CELL is created in the call context of writer (call context C in the description above) and awaited in the call context of reader (call context D in the description above). Calling the method go on a local replica results in the unexpected case of the method reader (called from go) producing no response because its call context (D) didn't reply while the method writer (call context C) returns 666 (instead of 42 suggested by the code of writer) because the continuation after the await point in the method reader was executed in the call context of writer (C) and returned 666. The debug output produced by a local replica for the above canister code reads:

[Canister rwlgt-iiaaa-aaaaa-aaaaa-cai] writer
[Canister rwlgt-iiaaa-aaaaa-aaaaa-cai] reader
[Canister rwlgt-iiaaa-aaaaa-aaaaa-cai] task
[Canister rwlgt-iiaaa-aaaaa-aaaaa-cai] task
[Canister rwlgt-iiaaa-aaaaa-aaaaa-cai] returning 666 from reader?
[Canister rwlgt-iiaaa-aaaaa-aaaaa-cai] reader: Err((CanisterError, "No response"))
[Canister rwlgt-iiaaa-aaaaa-aaaaa-cai] writer: Ok((666,))

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@mraszyk mraszyk marked this pull request as draft April 26, 2023 18:09
@adamspofford-dfinity
Copy link
Contributor

adamspofford-dfinity commented Apr 27, 2023

This is a reversion of an intentionally implemented feature - users who want lazy execution given eager execution can wrap in an async block, but users who want eager execution given lazy execution are out of luck.

@mraszyk mraszyk changed the title lazy inter-canister calls fix: lazy CallFuture Apr 28, 2023
@mraszyk
Copy link
Contributor Author

mraszyk commented Apr 28, 2023

This is a reversion of an intentionally implemented feature - users who want lazy execution given eager execution can wrap in an async block, but users who want eager execution given lazy execution are out of luck.

@adamspofford-dfinity I see your point. On the other hand, the example I've now included in the Motivation section results in a rather unexpected behavior and it's not clear to me if it can't be exploited for code obfuscation. Could you please take a look?

@Dfinity-Bjoern
Copy link
Member

@adamspofford-dfinity this PR is one (out of at least two) possible way of dealing with the unexpected behavior, so we may choose to resolve it in a different way. But don't you agree that this is an issue that should be resolved? The fact that writer may return something other than 42 or that reader may return with no reply seems entirely unexpected from looking at the Rust code. It basically renders typing guarantees useless.

@adamspofford-dfinity
Copy link
Contributor

adamspofford-dfinity commented May 2, 2023

Ah, I hadn't known this was an issue (the original comment didn't have the explanation @Dfinity-Bjoern), and also looking closer, the original purpose of this behavior has been superseded by notify. So never mind.

src/ic-cdk/src/api/call.rs Outdated Show resolved Hide resolved
src/ic-cdk/src/api/call.rs Outdated Show resolved Hide resolved
src/ic-cdk/src/api/call.rs Outdated Show resolved Hide resolved
src/ic-cdk/src/api/call.rs Outdated Show resolved Hide resolved
@mraszyk mraszyk marked this pull request as ready for review May 9, 2023 11:05
src/ic-cdk/src/api/call.rs Outdated Show resolved Hide resolved
* Allow argument data to be borrowed

* have these been non-extern fns the entire time?
@lwshang lwshang enabled auto-merge (squash) May 12, 2023 14:14
@lwshang lwshang merged commit 259ece4 into main May 12, 2023
15 checks passed
@lwshang lwshang deleted the mraszyk/lazy-calls branch May 12, 2023 14:28
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.

None yet

4 participants