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: consider refactoring between per-flowTools operations and per-flow operations #9375

Open
erights opened this issue May 16, 2024 · 1 comment
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 these "global" per flow tools, and not specific to each flow definition! See below for alternative

At #9097 (comment) @mhofman writes

I think these methods should be scoped to a flow definition. In particular it's a mistake to call wakeAll() before all asyncFlow have been redefined.

IMO we should have the following:

  • prepareAsyncFlowTools would define an exo class kit for flow definitions. Each instance would hold the failures and eagerWakers stores in their state, and have the following 2 facets:

    • An "admin facet" would implement the wakeAll() and getFailures() methods. Maybe it could also implement a getFlowForOutcomeVow() method (as a shortcut, we may not need to scope to the definition)
    • An "internal facet" allowing the flow kit behavior methods to interact with the stores of the definition as needed.
  • prepareAsyncFlowTools would also define a heap WeakMap mapping async flow (kit) makers to their respective admin facet of the definition instance.

  • prepareAsyncFlowKit would use a provide pattern for an instance of the above async flow definition kit. On first definition, an instance would be created and stashed somewhere, either in the provided zone (likely using a name derived from the flow's tag), or maybe in a shared store in the outerZone (if we're ever interested in easily iterating all flow definitions).

    • Technically the kind of the async flow kit could be held in the state of the definition instance, but I don't believe we can make that happen easily with zones.
    • The behavior methods of the flow kit would close over the "internal facet" of the flow definition to interact with the failures and eagerWakers stores
  • prepareAsyncFlowKit would register in the WeakMap the "admin facet" of the flow definition to the maker of the flow kit.

    • it will pin the flow definition kit in memory while the maker is retained, but this whole approach already assumes fairly low cardinality of async flow definitions
  • asyncFlow would similarly associate the admin facet of the flow definition with the wrapperFunc

  • Finally, the singleton adminAsyncFlow can give access to the flow definition's admin facet through the WeakMap

This would also allow to automatically wakeAll on the flow's definition (similar to how the flow maker "restarts" the flow on instantiation.

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

mhofman commented May 16, 2024

Since this requires a change in the data model, this should be implemented before any deployment of async flow.

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

No branches or pull requests

2 participants