Skip to content

feat: Implement heartbeat support #2971

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

Closed
wants to merge 24 commits into from
Closed

Conversation

ninegua
Copy link
Member

@ninegua ninegua commented Dec 8, 2021

Closes #2728

  • Basic support for heartbeat as a system function
  • Proper async type and type checking of the heartbeat function
  • Test cases

@ninegua
Copy link
Member Author

ninegua commented Dec 8, 2021

At the moment it seems to work. But I am trying to change heartbeat to return () instead of async (), then I got this error:

+heartbeat.mo:12.16-12.31: type error [M0037], misplaced async expression; try enclosing in an async function

Not exactly sure how to proceed. So I'm asking help from @crusso

@crusso
Copy link
Contributor

crusso commented Dec 9, 2021

I pulled your branch locally and played around with changing the return type to (), but couldn't quite make it work.
The type-checking changes were easy, but it falls over in the backend for some inscrutible reason.

To do that, you'd really want the heartbeat to be a shared oneway function so your example would wind up looking something like this:

import Prim "mo:⛔";

actor {
  var count = 0;
  public shared func inc() : async () {
    count := count + 1;
    Prim.debugPrint("count = " # debug_show(count));
  };

/*
  system func heartbeat() : async () {
    if (count < 10) {
      Prim.debugPrint("heartbeat");
      ignore inc();
    }
  };
 */

  system shared func heartbeat() : () {
     ignore async {
      if (count < 10) {
        Prim.debugPrint("heartbeat");
        ignore inc();
      }
    }
  };

};

Looking at that now, I'm not sure that's preferable to just declaring the async () type and using a local function.

I think you're lowered code might actually fail to type check once you extend check_ir.mo to check the heartbeat expression. The function call probably needs an instantiation at type T.Any (there's a phantom parameter there).

@crusso
Copy link
Contributor

crusso commented Dec 9, 2021

I've made the IR typeable, but I'm still a little worried that this code will have the hearbeat message generating a response, even though the spec says a heartbeat message cannot reply or reject. I guess that might generate a silent trap at the end of the heartbeat message or maybe the ic-ref and the replica aren't actually enforcing this (or I misread the spec)

@crusso
Copy link
Contributor

crusso commented Dec 9, 2021

Note to self: the way to fix this is probably to introduce a new CallHeartbeat primitive that passes dummy success/fail continuations (instead of ones that reply/reject).

@crusso
Copy link
Contributor

crusso commented Dec 17, 2021

Another small issue with this PR is that I believe it always exports the hearbeat method, even if it has trivial work. That's probably not what we want since every Motoko canister will no opt in to hearbeat notification, stressing the heart as it were.

@crusso
Copy link
Contributor

crusso commented Dec 17, 2021

Apart from the issue of always registering a heartbeat, I think I've convinced myself that this code is actually ok. I was worried it was attempting to reply or reject, but in fact, it just calls the helper method and discards it, without every registering a reply/reject continations which is probably ok - the body will run asynchronously but never respond.

@crusso crusso marked this pull request as ready for review January 5, 2022 20:37
@crusso crusso requested a review from nomeata January 5, 2022 20:40
@crusso
Copy link
Contributor

crusso commented Jan 5, 2022

I've redone a bit of the lowering/codegen and added some documentation. @ninegua PTAL if you care, otherwise I'll defer to @chenyan-dfinity or @ggreif for review.

(This implementation will do an extra async call for each heartbeat, but that's really a problem with our implementation of async functions and will be fixed by fixing that. I didn't have the stomach to circumvent this by adding a hack. An alternative would be to also support ()-returning heartbeats that are executed inline, but cannot send, but that seems gross too).

@crusso
Copy link
Contributor

crusso commented Jan 5, 2022

@ninegua I seem to have confused git with my inept merging of branches - sorry about that.

@crusso
Copy link
Contributor

crusso commented Jan 6, 2022

PR moved to #3039 to improve perf of CI

@ggreif
Copy link
Contributor

ggreif commented Jan 10, 2022

As #3039 has landed, I think this can be closed now.

@ggreif
Copy link
Contributor

ggreif commented Jan 10, 2022

Please reopen if something is still missing.

@ggreif ggreif closed this Jan 10, 2022
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.

Support the heartbeat
3 participants