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

async-flow: postpone wakeAll after first crank rather than first turn. #9377

Open
erights opened this issue May 16, 2024 · 3 comments · May be fixed by #10208
Open

async-flow: postpone wakeAll after first crank rather than first turn. #9377

erights opened this issue May 16, 2024 · 3 comments · May be fixed by #10208
Assignees
Labels
asyncFlow related to membrane-based replay and upgrade of async functions bug Something isn't working

Comments

@erights
Copy link
Member

erights commented May 16, 2024

At #9097 (comment) @mhofman writes

I just realized [a call to wakeAll] was commented out. I think it'll be a footgun if the flows are not awaken automatically.

because the wakeAll can only happen correctly after all prepares have been done. Thus, it should be postponed until after the first crank. But, as it currently reads:

  // Cannot call this until everything is prepared, so postpone to a later
  // turn. (Ideally, we'd postpone to a later crank because prepares are
  // allowed anytime in the first crank. But there's currently no pleasant
  // way to postpone to a later crank.)
  const allWokenP = E.when(null, () => adminAsyncFlow.wakeAll());

We now demonstrate this bug with the new async-flow-crank.test.js

@erights erights added bug Something isn't working asyncFlow related to membrane-based replay and upgrade of async functions labels May 16, 2024
@mhofman
Copy link
Member

mhofman commented May 17, 2024

it should be postponed until after the first crank

I don't think it should be postponed until after the first crank. In particular we may in the future want to define a flow such that the vat upgrade fails if the replay of existing flows fails. That would require existing flows to be awoken in the first start crank.

the wakeAll can only happen correctly after all prepares have been done

There are 2 solutions to this:

  • Keep the list of flows to wake up to be global, but trigger wakeAll after all flows have been redefined. That would require the flow tools to durably keep track of which flows had been defined, and only invoke wakeAll after the last one is redefined. Note you cannot use a simple counter because a new incarnation may define a new flow before a previous flow is redefined.
  • implement async-flow: consider refactoring between per-flowTools operations and per-flow operations #9375 and instead wake the flows specific to each definition immediately after they are redefined.

@mhofman
Copy link
Member

mhofman commented Jul 22, 2024

We recently realized that liveslots currently enforces that all kinds are reconnected by the time buildRootObject settles. For ZCF contracts, we're not providing a helper to wrap the start function providing asyncFlow. If the kinds defined by ZCF itself are defined before start is called, we could have this start wrapper call wakeAll after start's settlement, as we'd be relying on expected behavior that the contract reconnected all kinds.

@mhofman
Copy link
Member

mhofman commented Aug 2, 2024

If we rely on the caller of prepareAsyncFlowTools to call adminAsyncFlow.wakeAll(), we likely should enforce or warn if it hasn't been called by the time an invocation is created or woken up.

@mhofman mhofman linked a pull request Oct 3, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
asyncFlow related to membrane-based replay and upgrade of async functions bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants