handle floating promises in governance #5584
Conversation
| @@ -10,7 +10,7 @@ import { Far } from '@endo/marshal'; | |||
| /** @type {CloseVoting} */ | |||
| export const scheduleClose = (closingRule, closeVoting) => { | |||
| const { timer, deadline } = closingRule; | |||
| E(timer).setWakeup( | |||
| return E(timer).setWakeup( | |||
There was a problem hiding this comment.
| return E(timer).setWakeup( | |
| void E(timer).setWakeup( |
CloseVoting claims to have return type void
| @@ -228,10 +228,16 @@ const makeBootstrap = (argv, cb, vatPowers) => async (vats, devices) => { | |||
| const [testName] = argv; | |||
| switch (testName) { | |||
| case 'committeeBinaryStart': | |||
| committeeBinaryStart(zoe, voterCreator, timer, log, installations); | |||
| await committeeBinaryStart(zoe, voterCreator, timer, log, installations); | |||
There was a problem hiding this comment.
I don't think await is doing anything helpful here. Wouldn't void be clearer?
[same below]
There was a problem hiding this comment.
It's preserving the stack trace if committeeBinaryStart fails. Without these awaits, makeBootstrap will complete as soon as the case functions are called. If they fail it will throw in Node as an unhandled promise rejection.
I think if they fail then makeBootstrap should also fail.
| @@ -43,7 +43,7 @@ const build = async (log, zoe) => { | |||
| }, | |||
| }); | |||
| const subscription = E(electoratePublicFacet).getQuestionSubscription(); | |||
| observeIteration(subscription, votingObserver); | |||
| await observeIteration(subscription, votingObserver); | |||
There was a problem hiding this comment.
Similarly here. Isn't await misleading? The point of the code is to set the observer off on it's own, and return the facet. Why is await better?
There was a problem hiding this comment.
Yeah, I was on the fence about this case. The point is to kick off the observer and not expect anything back, but what if it fails? Like above, awaiting maintains the stack trace.
If we don't want to detect that failure, then I think we should change observeIteration (the called function) to be synchronous instead of dropping the promise in every call.
From
agoric-sdk/packages/notifier/src/asyncIterableAdaptor.js
Lines 135 to 140 in cc522ee
*/
export const observeIteration = (asyncIterableP, iterationObserver) => {
const iteratorP = E(asyncIterableP)[Symbol.asyncIterator]();
void observeIterator(iteratorP, iterationObserver);
};Which do you think is better?
There was a problem hiding this comment.
Attempting to channel @erights: I expect some callers to want to be able to react to the results. In this test, it seems that the test should be conditional on results of the observation, so it doesn't really need to detect a failure in the adapter.
| @@ -390,7 +390,7 @@ const makeBootstrap = (argv, cb, vatPowers) => async (vats, devices) => { | |||
|
|
|||
| await E(timer).tick(); | |||
| await E(timer).tick(); | |||
| E.when( | |||
| await E.when( | |||
There was a problem hiding this comment.
| await E.when( | |
| void E.when( |
The test doesn't want to wait here. It wants these logs requests to fire later.
There was a problem hiding this comment.
thanks, I was wondering why they were failing ;-)
| @@ -443,7 +443,7 @@ const makeBootstrap = (argv, cb, vatPowers) => async (vats, devices) => { | |||
|
|
|||
| await E(timer).tick(); | |||
| await E(timer).tick(); | |||
| E.when( | |||
| await E.when( | |||
There was a problem hiding this comment.
| await E.when( | |
| void E.when( |
turadg
force-pushed
the
ta/governance-promises
branch
from
9f49ec6 to
1563e55
Compare
turadg
force-pushed
the
ta/governance-promises
branch
from
1563e55 to
42e0c03
Compare
no ticket, busy hands during standup
Description
Handle floating promises.
Security Considerations
--
Documentation Considerations
--
Testing Considerations
CI