fix(notifier): Make makeNotifierFromAsyncIterable lossy #5695
Conversation
| ); | ||
| const update1 = await notifier.getUpdateSince(); | ||
| const governorSubscription = await E(publicFacet).getSubscription(); | ||
| const governorUpdates = await E(governorSubscription)[Symbol.asyncIterator](); |
There was a problem hiding this comment.
I'm not clear on why such use of makeNotifierFromAsyncIterable for testing in governance and run-protocol causes lockup (e.g., https://github.com/Agoric/agoric-sdk/runs/7138160731?check_suite_focus=true : "unitTests › paramGovernance › change multiple params … Error: Promise returned by test never resolved"). It could be related to use of a fake timer, or could be an actual bug. But the tests don't appear to have been expecting lossyness, so I feel mostly fine about direct use of await … next().
There was a problem hiding this comment.
I'm concerned that we don't understand why it's failing. If it's isolated to these cases then I can agree to sweep it under the rug as something not worth getting to the bottom of.
I'm not sure it's isolated though. #5704 attempts to use makeNotifierFromAsyncIterable for notifiers (necessary for #5386 ) and it also hangs the same way.
| next: () => { | ||
| const resultP = E.get(tailP).head; | ||
| next: async () => { | ||
| const resultP = await E.get(tailP).head; |
There was a problem hiding this comment.
This one might not be necessary, but it does seem problematic to eagerly to let an eager consumer (such as the new makeNotifierFromAsyncIterable) read an arbitrarily long sequence of results that might never materialize.
There was a problem hiding this comment.
Recapping from our meeting per request:
- leave
makeNotifierFromAsyncIterableas is - deprecate it because it's a hot notifier (to be removed in remove makeNotifierFromAsyncIterable from vat-timer #5709 )
- make something new that takes a subscriber to make a cold notifier
|
Superseded by #5737. |
Eager consumption led to infinite loops; see #5695 for context.
Eager consumption led to infinite loops; see #5695 for context.
* 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>
* 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>
Fixes #5413
Description
Separate iterable consumption from
getUpdateSincepeeking.Security Considerations
Because the consumption is now greedy, there is a risk of getting stuck in a produce–consume pattern where each step enqueues a subsequent turn and forward progress halts. But I don't think any of our code fits that pattern, and at any rate I don't know if any mitigation is even possible.
Documentation Considerations
n/a
Testing Considerations
WIP