chore: resolve floating-promise warnings in kernel packages #8214

toliaqat · 2023-08-17T07:52:27Z

closes: #8210
refs: #6000

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

AGORIC_ESLINT_TYPES=true yarn lint

Upgrade Considerations

warner

Looks good.. modulo the then(.., noop) questions, I think the code changes are fine.

In the future, we may need to cherry-pick portions of this for selective deployment purposes. Could you split the PR into multiple commits, to avoid co-mingling changes to packages that may need to be deployed separately?

one commit for internal
one commit for SwingSet and swingset-liveslots
one for xsnap
one for cosmic-swingset and solo

I'd recommend waiting for @mhofman to chime in, then both split it into four commits and rebase it onto current trunk. After that, feel free to land.

warner · 2023-08-22T18:37:52Z

packages/xsnap/src/xsnap.js

@@ -152,7 +152,7 @@ export async function xsnap(options) {
      sourceStream.pipe(destStream, { end: false });

      done = finished(sourceStream);
-      done.catch(noop).then(() => sourceStream.unpipe(destStream));
+      done.catch(noop).then(() => sourceStream.unpipe(destStream), noop);


Huh, I'm guessing we needed the errback (second arg) in the .then() because the linter doesn't believe that noop will never throw? But, why didn't it complain that the promise returned by .then is similarly ungrounded?

@mhofman is the expert on this code, but I'm guessing that a void in front of the whole thing would be reasonable, and if that addresses whatever linter complaint that prompted the extra noop inside the then(), it might be a better fix overall.

That extra noop doesn't change a thing since we already have a .catch(noop) before that. And yeah it's more likely the linter would complain about sourceStream.unpipe(destStream) throwing (which it doesn't in practice). We can void the resulting promise if needed in this case.

warner · 2023-08-22T18:38:21Z

packages/xsnap/src/xsnap.js

@@ -264,7 +264,7 @@ export async function xsnap(options) {
        loadSnapshotHandler = undefined;
        return cleanup();
      }
-    });
+    }, noop);


probably the same issue here

Same indeed

warner · 2023-08-22T18:38:58Z

packages/xsnap/src/xsnap.js

@@ -470,7 +470,7 @@ export async function xsnap(options) {
        // ensuring that any previous save stream usage has ended. However we
        // must start the flow before receiving the command's response or the
        // xsnap process would block on a full pipe, causing an IPC deadlock.
-        batonKit.promise.then(maybePipe);
+        batonKit.promise.then(maybePipe, noop);


maybe the same issue here? I definitely want @mhofman to drive this part, I don't understand the whole "baton" thing very well

This one is valid, it silences a potential rejection of the baton that would bottom out somewhere else.

warner · 2023-08-22T18:40:59Z

packages/xsnap/src/xsrepl.js

@@ -97,4 +97,4 @@ async function main() {
  return vat.close();
 }

-main();
+await main();


Oh, right, can we use top-level await now? Nice, I haven't internalized that yet.

I'd go one step further and do await main().catch(err => console.log(err)), because in my experience, simple errors or typos in functions called like this main() get lost, and the explicit catch is the only way to discover the problem.

mhofman

Thanks for tackling these. Just a couple adjustments needed.

I'd like to have @michaelfig review the solo part (I didn't review)

mhofman · 2023-08-22T20:06:01Z

packages/xsnap/src/xsnap.js

@@ -152,7 +152,7 @@ export async function xsnap(options) {
      sourceStream.pipe(destStream, { end: false });

      done = finished(sourceStream);
-      done.catch(noop).then(() => sourceStream.unpipe(destStream));
+      done.catch(noop).then(() => sourceStream.unpipe(destStream), noop);


That extra noop doesn't change a thing since we already have a .catch(noop) before that. And yeah it's more likely the linter would complain about sourceStream.unpipe(destStream) throwing (which it doesn't in practice). We can void the resulting promise if needed in this case.

mhofman · 2023-08-22T20:06:16Z

packages/xsnap/src/xsnap.js

@@ -264,7 +264,7 @@ export async function xsnap(options) {
        loadSnapshotHandler = undefined;
        return cleanup();
      }
-    });
+    }, noop);


Same indeed

mhofman · 2023-08-22T20:07:30Z

packages/xsnap/src/xsnap.js

@@ -458,10 +458,10 @@ export async function xsnap(options) {
        const handle = await fs.open(snapPath, 'w+');
        // @ts-expect-error 'close' event added in Node 15.4
        handle.on('close', () => {
-          fs.unlink(snapPath);
+          void fs.unlink(snapPath);


Maybe add a comment in this case that it's safe to ignore the result because it simply means we may not have cleaned up a temp directory.

mhofman · 2023-08-22T20:11:04Z

packages/xsnap/src/xsnap.js

@@ -470,7 +470,7 @@ export async function xsnap(options) {
        // ensuring that any previous save stream usage has ended. However we
        // must start the flow before receiving the command's response or the
        // xsnap process would block on a full pipe, causing an IPC deadlock.
-        batonKit.promise.then(maybePipe);
+        batonKit.promise.then(maybePipe, noop);


This one is valid, it silences a potential rejection of the baton that would bottom out somewhere else.

mhofman · 2023-08-22T20:12:01Z

packages/xsnap/src/xsnap.js

        });
        sourceStream = handle.createReadStream();
-        finished(output).finally(() => sourceStream.destroy());
+        void finished(output).finally(() => sourceStream.destroy());


so this in an interesting case of unhandled promise. In this case I believe we want to silence any error that may occur.

Suggested change

void finished(output).finally(() => sourceStream.destroy());

finished(output).finally(() => sourceStream.destroy()).catch(noop);

FUDCo

I second @warner's suggestion to use separate commits for separable stuff, but other than that it looks good. Hoorah for taking out the trash!

michaelfig

I reviewed the packages/solo changes, and they LGTM!

toliaqat force-pushed the 8210-tl-resolve-floating-promises branch from 786e96f to b585fe9 Compare August 17, 2023 08:09

toliaqat marked this pull request as ready for review August 17, 2023 18:01

toliaqat requested review from warner, mhofman and FUDCo August 17, 2023 18:01

warner approved these changes Aug 22, 2023

View reviewed changes

mhofman reviewed Aug 22, 2023

View reviewed changes

FUDCo approved these changes Aug 22, 2023

View reviewed changes

michaelfig approved these changes Aug 23, 2023

View reviewed changes

toliaqat force-pushed the 8210-tl-resolve-floating-promises branch 3 times, most recently from 32002d8 to e852eeb Compare August 25, 2023 05:59

mhofman approved these changes Aug 28, 2023

View reviewed changes

warner added the automerge:rebase Automatically rebase updates, then merge label Aug 28, 2023

toliaqat added 6 commits August 28, 2023 19:39

chore(internal): resolve floating-promise warnings

bf0fdcc

chore(solo): resolve floating-promise warnings

a521a9a

chore(swingset-liveslots): resolve floating-promise warnings

e5adec5

chore(SwingSet): resolve floating-promise warnings

39b9323

chore(cosmic-swingset): resolve floating-promise warnings

6439256

chore(xsnap): resolve floating-promise warnings

f5fb0ce

toliaqat force-pushed the 8210-tl-resolve-floating-promises branch from e852eeb to f5fb0ce Compare August 28, 2023 19:39

mergify bot merged commit 0ed9127 into master Aug 28, 2023

mergify bot deleted the 8210-tl-resolve-floating-promises branch August 28, 2023 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: resolve floating-promise warnings in kernel packages #8214

chore: resolve floating-promise warnings in kernel packages #8214

toliaqat commented Aug 17, 2023 •

edited

Loading

warner left a comment

warner Aug 22, 2023

mhofman Aug 22, 2023

warner Aug 22, 2023

mhofman Aug 22, 2023

warner Aug 22, 2023

mhofman Aug 22, 2023

warner Aug 22, 2023

mhofman left a comment

mhofman Aug 22, 2023

mhofman Aug 22, 2023

mhofman Aug 22, 2023

mhofman Aug 22, 2023

mhofman Aug 22, 2023

FUDCo left a comment

michaelfig left a comment

	void finished(output).finally(() => sourceStream.destroy());
	finished(output).finally(() => sourceStream.destroy()).catch(noop);

chore: resolve floating-promise warnings in kernel packages #8214

chore: resolve floating-promise warnings in kernel packages #8214

Conversation

toliaqat commented Aug 17, 2023 • edited Loading

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

warner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FUDCo left a comment

Choose a reason for hiding this comment

michaelfig left a comment

Choose a reason for hiding this comment

toliaqat commented Aug 17, 2023 •

edited

Loading