TimerService has been revised, documentation hasn't kept up

[Chris-Hibbert]

From a recent interchange between @warner and @Chris-Hibbert. This is not complete documentation, but it's part of what needs to be captured. I told Brian that my when I used repeatAfter, my handler was only getting called once and asked for help diagnosing my issue. I observed that the new APIs hadn't been documented sufficiently, and Brian agreed.

repeatAfter takes a handler with a .wake() method that is allowed to return a Promise, and the timer won't get rescheduled until that promise fulfills. If it rejects (e.g. if the handler's hosting vat has died) or hangs, the timer won't keep pinging. https://github.com/Agoric/agoric-sdk/blob/master/packages/SwingSet/src/vats/timer/vat-timer.js#L637-L644 basically describes the API. In general, the new implementation is more strict about rescheduling: to minimize eternal workloads, it cancels more aggressively than before, and requires positive affirmation (even just an empty result promise getting fulfilled to undefined) to continue.

  /**
   * Internal function to register a handler, which will be invoked as
   * handler.wake(scheduledTime) at the earliest non-past instance of
   * `start + k*interval`. When the wake() result promise
   * fulfills, the repeater will be rescheduled for the next such
   * instance (there may be gaps). If that promise rejects, the
   * repeater will be cancelled. The repeater can also be cancelled by
   * providing `cancelToken` and calling
   * `E(timerService).cancel(cancelToken)`.
   */

I asked

What code detects the fulfilled promise?
Is it enough to fall off the end of the handler without throwing, or does it have to return true or something?

https://github.com/Agoric/agoric-sdk/blob/master/packages/SwingSet/src/vats/timer/vat-timer.js#L512-L518

      E(state.handler)
        .wake(now)
        .then(
          _res => self.wakerDone(), // reschedule unless cancelled
          _err => self.wakerFailed(), // do not reschedule

it sends wake(), and puts a .then on the result
so a wake() that isn't async and doesn't throw will always reschedule
or a wake() that is async but never awaits (and doesn't throw) (edited)
but a wake() that does return new Promise(() => 0) (i.e. hangs) will not get rescheduled (it will continue to consume RAM in vat-timer, but it won't consume any extra CPU time) (edited)

Brian then wrote:

any thoughts about how to make this more visible in the future? I thought about adding a log to wakerFailed but it seemed like it might be noisy.

I responded

If it's noisy, then someone's making a mistake.
I think a log would be very helpful. I can't think why it would be noisy if used properly.

To diagnose my problem, I changed the err branch above to

          err => {
            console.log(`VATT  fired.catch`, err);
            self.wakerFailed();
          }, // do not reschedule

Brian added issue #6767.

[dckc]

@amessbee nice work on https://github.com/amessbee/dapp-chain-timer/ ; I suggest we re-scope this issue based on it...

I suggest moving it under agoric-labs and linking it from relevant part of the docs.

Add a TimerService glossary entry while you're at it, please, since it's such a distinctive benefit of our orchestration platform.