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

PubSub #1949

Merged
merged 26 commits into from
Nov 2, 2020
Merged

PubSub #1949

merged 26 commits into from
Nov 2, 2020

Conversation

erights
Copy link
Member

@erights erights commented Oct 30, 2020

Fixes #1345

Supports a lossless stream of values through the standard JavaScript async iterable API, in a remotable manner.

@erights erights self-assigned this Oct 30, 2020
@erights erights marked this pull request as ready for review October 30, 2020 19:55
packages/marshal/marshal.js Show resolved Hide resolved
packages/notifier/src/asyncIteratableKit.js Outdated Show resolved Hide resolved
packages/notifier/src/asyncIteratableKit.js Outdated Show resolved Hide resolved
packages/notifier/src/asyncIteratableKit.js Outdated Show resolved Hide resolved
packages/notifier/src/asyncIteratableKit.js Outdated Show resolved Hide resolved
@erights
Copy link
Member Author

erights commented Nov 1, 2020

Hi @kriskowal, @Chris-Hibbert , ready for review.

Naming is hard. I renamed everything as @kriskowal and I discussed over zoom. Please read the README first to react to the new names. I'm not in love with them.

Once the only remaining problem is settling the names, unless there's an obvious and agreed choice, I would prefer to land this PR first so that the name bikeshedding can then proceed at its own pace. ERTP and Zoe went through several major naming changes before settling down.

@kriskowal kriskowal changed the title WIP async iterable kit PubSub Nov 1, 2020
@kriskowal
Copy link
Member

I’ve taken the presumptuous liberty of renaming the PR “PubSub”, not so much because that’s clearly the right name, but because this is now in earnest review, and to avoid committing with a message with “WIP”. That would have left “async iterable kit”, which isn’t representative.

@michaelfig
Copy link
Member

michaelfig commented Nov 1, 2020

@erights I like the direction this is going, but it's missing one use case that I still have that I discussed with @kriskowal but unfortunately not with you.

I'd like an async iterator that produces these kinds of streams:

0a-0b-0c-0d-1snap-1a-1b-1c-1d-2snap-3snap-3a-3b-3c-finalsnap

The idea is that a recent client state can be built from the latest *snap update plus the *[a-z] deltas that follow it. Whenever a new Nsnap is produced, all the producer's prior history can be dropped without fear of dropping information a future client will need.

I want this for long-lived produces so that they don't consume arbitrary memory, reducing their history only to the latest snap+deltas. Consumers can still be assured that they get a lossless value by requesting the latest snapshot and following the stream from there.

This seems to be the more general underlying hybrid mechanism from which lossless:

0a-0b-0c-0d-0e-0f-...

and lossy:

1snap-2snap-3snap-4snap-5snap-...

can be built.

Thoughts?

(@warner @dckc thought you might like the isomorphism with recovering a vat's state from kernel snapshots + deltas.)

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider your response to my snapshots+deltas model #1949 (comment) before merging.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there’s a coherent story for @michaelfig’s idea should we add a solution for that case, commented inline.

I don’t suppose we could also have test cases where the producer and consumer are genuinely in separate vats, to give eventual send some inter-vat exercise?

packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Outdated Show resolved Hide resolved
packages/notifier/README.md Show resolved Hide resolved
packages/notifier/src/subscriber.js Show resolved Hide resolved
packages/notifier/src/types.js Outdated Show resolved Hide resolved
@erights
Copy link
Member Author

erights commented Nov 2, 2020

@michaelfig , In response to your suggestion at #1949 (comment) , please take a look at https://github.com/Agoric/agoric-sdk/blob/async-iter-kit/packages/notifier/test/map-unum.js .

This experiment proposes that the answer to @michaelfig's question is to nest lossless delta subscriptions inside lossy snapshot notifiers.

Though note the rest of the disclaimer at the top.

Comment on lines +135 to +145
Although Bob's code is correct, it is sub-optimal. Its distributed systems
properties are not terrible, but Bob can do better using the
`getSharableSubsciptionInternals()` method provided by both NotifierKit and
SubscriptionKit. This enables consumer Bob to make a local AsyncIterable that
coordinates better with producer Paula's IterationObserver. Alice's code is
less forgiving. She's using JavaScript's `for-await-of` loop which requires a
local AsyncIterable. It cannot handle a remote reference to an AsyncIterable
at
Paula's site. Alice has no choice but to make an AsyncIterable at her site.
Using `getSharableSubsciptionInternals()` is the best way for her to do so. She
can replace her last line, the call to `consume(subscription)` with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering whether there might be a way to hide this concern from users. This is a lot to chew.

These docs might benefit from being overall driven by a sequence of motivating use cases with concrete domains, like the current time, progress, ETA, absolute values vs deltas.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering whether there might be a way to hide this concern from users. This is a lot to chew.

There is --- building more direct support for Unum replication into marshal and the comm systems built on it. However, it requires some kind of name-based code registry be privileged. This also comes up for the ERTP MathHelpers. We don't yet have a worked out design for reconciling such a name-based registry with permissionless-ness, so we have postponed the issue. But we know we'll need this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These docs might benefit from being overall driven by a sequence of motivating use cases with concrete domains, like the current time, progress, ETA, absolute values vs deltas.

They would indeed, but please not in this PR. For now I'm trying only for precision and a certain basic level of readability. Does it seem adequate to merge in its current state?

@michaelfig michaelfig self-requested a review November 2, 2020 03:22
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm loving map-unum.js! An independently-discovered by @erights and @kriskowal elegant composition of the tools at hand to solve the case I had in mind.

Hence, this PR LGTM.

@erights
Copy link
Member Author

erights commented Nov 2, 2020

I'm loving map-unum.js! An independently-discovered by @erights and @kriskowal elegant composition of the tools at hand to solve the case I had in mind.

Hence, this PR LGTM.

Thanks for the kind words! It was an interesting puzzle. Very gratifying.

@erights erights merged commit 716b363 into master Nov 2, 2020
@erights erights deleted the async-iter-kit branch November 2, 2020 03:50
@katelynsills
Copy link
Contributor

This looks awesome! We will need some way of getting this documentation out to the users so they know what is available and how to use it. @erights could you put a link in the documentation repo or coordinate with Tom to do so?

@erights
Copy link
Member Author

erights commented Nov 2, 2020

Thanks!

Already sent @tyg email

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.

Marshal needs some support for invoking symbol-named methods
5 participants