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.

Sign up for GitHub

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

Jump to bottom

notifier returned by makeNotifierFromAsyncIterable is lossless #5413

Closed
turadg opened this issue May 20, 2022 · 0 comments · Fixed by #5737
Closed

notifier returned by makeNotifierFromAsyncIterable is lossless #5413

turadg opened this issue May 20, 2022 · 0 comments · Fixed by #5737
Assignees
@erights @gibson042
Labels
bug Something isn't working notifier
Milestone
Mainnet 1 RC0

Comments

@turadg
Copy link
Member

turadg commented May 20, 2022 •
edited
Loading

Describe the bug

The design of Notifier is that if you call getUpdateSince() without any arguments, you get the latest state.

This is different for the alleged Notifier returned by makeNotifierFromAsyncIterable. It never skips elements of the iteration of the underlying subscription.

Expected behavior

A Notifier skips states consistently. Either makeNotifierFromAsyncIterable conforms or is renamed. (Or replaced if there's something better.)
@turadg turadg added the bug Something isn't working label May 20, 2022
@turadg turadg added the SwingSet package: SwingSet label Jun 1, 2022
@warner warner added notifier and removed SwingSet package: SwingSet labels Jun 8, 2022
gibson042 added a commit that referenced this issue Jun 30, 2022
@gibson042
fix(notifier): Make makeNotifierFromAsyncIterable lossy
28390a9 
Fixes #5413
@gibson042 gibson042 mentioned this issue Jun 30, 2022
Closed
gibson042 added a commit that referenced this issue Jul 8, 2022
@gibson042
fix(notifier): Cherry-pick makeAsyncIterableFromNotifier improvements
4c33477 
gh-5413-lossy-makeNotifierFromAsyncIterable
#5695
gibson042 added a commit that referenced this issue Jul 8, 2022
@gibson042
feat(notifier): Add makeNotifierFromSubscriber
e52367c 
Fixes #5413
@gibson042 gibson042 mentioned this issue Jul 8, 2022
Merged
gibson042 added a commit that referenced this issue Jul 8, 2022
@gibson042
fix(notifier): Make makeAsyncIterableFromNotifier lossy
820e340 
Cherry-picked from gh-5413-lossy-makeNotifierFromAsyncIterable.
See #5695 .
gibson042 added a commit that referenced this issue Jul 8, 2022
@gibson042
feat(notifier): Add makeNotifierFromSubscriber
9d6f025 
Fixes #5413
gibson042 added a commit that referenced this issue Jul 8, 2022
@gibson042
fix(notifier): Make makeAsyncIterableFromNotifier lossy
8ff8718 
Cherry-picked from gh-5413-lossy-makeNotifierFromAsyncIterable.
See #5695 .
gibson042 added a commit that referenced this issue Jul 8, 2022
@gibson042
feat(notifier): Add makeNotifierFromSubscriber
e92402c 
Fixes #5413
turadg pushed a commit that referenced this issue Jul 8, 2022
@gibson042 @turadg
feat(notifier): Add makeNotifierFromSubscriber
6d521a8 
Fixes #5413
turadg pushed a commit that referenced this issue Jul 8, 2022
@gibson042 @turadg
feat(notifier): Add makeNotifierFromSubscriber
8c15f8e 
Fixes #5413
turadg added a commit that referenced this issue Jul 12, 2022
@turadg
Merge branch 'gh-5413-lossy-makeNotifierFromAsyncIterable' into ta/va…
679771f 
…ult-off-chain
mergify bot added a commit that referenced this issue Jul 13, 2022
@mergify
Merge branch 'master' into gh-5413-makeNotifierFromSubscriber
a84d6c8
@mergify mergify bot closed this as completed in #5737 Jul 14, 2022
mergify bot added a commit that referenced this issue Jul 14, 2022
@gibson042 @mergify @turadg
feat(notifier): Add makeNotifierFromSubscriber (#5737)
077718a 
* fix(notifier): Make makeAsyncIterableFromNotifier lossy

Cherry-picked from gh-5413-lossy-makeNotifierFromAsyncIterable.
See #5695 .

* fix(notifier): Revert "Make makeAsyncIterableFromNotifier lossy"

Eager consumption led to infinite loops; see
#5695 for context.

* feat(notifier): Add makeNotifierFromSubscriber

Fixes #5413

* test(notifier): Update per code review

* chore(notifier): Resolve lint warnings

* fix(notifier): Align makeNotifierFromSubscriber with makeNotifierKit

getUpdateSince() always consults the source subscribeAfter() rather than
using a possibly-stale local cache.

* fix master merge

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Turadg Aleahmad <turadg@agoric.com>
@turadg turadg mentioned this issue Jul 14, 2022
Closed
turadg added a commit that referenced this issue Jul 14, 2022
@gibson042 @mergify @turadg
feat(notifier): Add makeNotifierFromSubscriber (#5737)
dc1c574 
* fix(notifier): Make makeAsyncIterableFromNotifier lossy

Cherry-picked from gh-5413-lossy-makeNotifierFromAsyncIterable.
See #5695 .

* fix(notifier): Revert "Make makeAsyncIterableFromNotifier lossy"

Eager consumption led to infinite loops; see
#5695 for context.

* feat(notifier): Add makeNotifierFromSubscriber

Fixes #5413

* test(notifier): Update per code review

* chore(notifier): Resolve lint warnings

* fix(notifier): Align makeNotifierFromSubscriber with makeNotifierKit

getUpdateSince() always consults the source subscribeAfter() rather than
using a possibly-stale local cache.

* fix master merge

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Turadg Aleahmad <turadg@agoric.com>
@Tartuffo Tartuffo added this to the Mainnet 1 milestone Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@erights erights

@gibson042 gibson042

Labels
bug Something isn't working notifier
Projects
None yet
Milestone
Mainnet 1 RC0
5 participants
@turadg @warner @erights @Tartuffo @gibson042