`makeCapTP()` accepts async `send()` functions, but does not handle rejected promises

[rekmarks]

The current implementation of makeCapTP() expects a synchronous send(). Yet, in e.g. the daemon, a Promise-returning send() is provided. Any rejections by this function will currently be uncaught. It seems reasonable that consumers should be able to provide Promise-returning send() functions, so this should probably be fixed in @endo/captp as opposed to its consumers.

[erights]

That first link https://github.com/endojs/endo/blob/aaec18819a143aae62d898f08a23027e3ad0d2dd/packages/captp/src/captp.js#L66 links to a line that says

* @param {(obj: Record<string, any>) => void} rawSend send a JSONable packet

IIUC, the TS meaning of void return is different than undefined return. A TS undefined return says the function does return the JS undefined value, which async functions do not. A TS void return says "don't depend on what this function returns". That would be compatible with the function being sync or async, and so does not seem incorrect. And indeed both calls to rawSend ignore its return result, which is consistent with the void return typing and with it being an async function.

OTOH, one of those calls is preceded by a comment that might be confusing. At https://github.com/endojs/endo/blob/aaec18819a143aae62d898f08a23027e3ad0d2dd/packages/captp/src/captp.js#L196-L197

    // Actually send the message, in the next turn.
    rawSend(obj);

The call immediately below that comment is a synchronous call. Even if the function is async, the call is still sync, and the body of the function starts while this call is suspended underneath it on the stack. Presumably, this comment is not actually about whether rawSend is sync or async, but rather about what rawSend is supposed to do. Yes?

Attn @michaelfig

[erights]

Any rejections by this function will currently be uncaught

This is the core issue being raised here, right? For this it doesn't matter whether the function is sync or async. It merely matters how the caller is supposed to create if rawSend returns a promise which gets rejected. Yes?

[rekmarks]

Any rejections by this function will currently be uncaught

This is the core issue being raised here, right? For this it doesn't matter whether the function is sync or async. It merely matters how the caller is supposed to create if rawSend returns a promise which gets rejected. Yes?

Yes, correct. I have updated the issue title to better reflect this.

You are also correct that the types are currently valid, and that e.g. async () => undefined can be assigned to a variable of type () => void. However, for functions that may or may not be async, the return type void | Promise<void> would be more appropriate. I opened this issue after the @typescript-eslint rule no-misused-promises complained about passing an async send() to makeCapTP() in a MetaMask repo.

For example, the rule is unhappy about this:

const foo: () => void = async () => undefined;
// error: Promise-returning function provided to variable where a void return was expected

But content with either of these:

const func = async () => undefined;

const syncOrAsync: () => void | Promise<void> = func;
const asyncOnly: () => Promise<void> = func;

[michaelfig]

A bit of background:

@endo/captp is designed to

1. abort the connection if there is a synchronous failure during dispatch, where rawSend() is wrapped by a synchronous try-catch, and
2. return promise rejections created from synchronous exceptions during the handler methods, where rawSend throws synchronously during the HandledPromise[string] static methods ('applyMethod', 'applyFunction', 'get', etc.).

Summarising my understanding (thanks @rekmarks and @erights):

To do the minimal work needed to honour rawSend's rejections, a. rawSend would be typed as (obj: Record<string, any>) => void | PromiseLike<void>, b. send would become an async function and await rawSend(obj) would be used (but not returned from send, so that only rejections are propagated), c. dispatch, and the handler methods would become async, and await send(obj) would be used to propagate send's rejections to the 1.try-catch, or 2.caller of HandledPromise static methods.

I don't see any reason to make the error handling more fine-grained than that. I'd also gladly review a PR that makes this happen, with some tests that show the new functionality (and hopefully does not disrupt existing usage).

[michaelfig]

Oh, I had another thought. There's a much less invasive change that also preserves the existing behaviour:

diff --git a/packages/captp/src/captp.js b/packages/captp/src/captp.js
index cfb9da8e2..6951a8ae2 100644
--- a/packages/captp/src/captp.js
+++ b/packages/captp/src/captp.js
@@ -63,7 +66,7 @@ const reverseSlot = slot => {
  * Create a CapTP connection.
  *
  * @param {string} ourId our name for the current side
- * @param {(obj: Record<string, any>) => void} rawSend send a JSONable packet
+ * @param {(obj: Record<string, any>) => void | PromiseLike<void>} rawSend send a JSONable packet
  * @param {any} bootstrapObj the object to export to the other side
  * @param {CapTPOptions} opts options to the connection
  */
@@ -193,8 +196,10 @@ export const makeCapTP = (
       return;
     }

-    // Actually send the message, in the next turn.
-    rawSend(obj);
+    // Actually send the message.
+    Promise.resolve(rawSend(obj))
+      // eslint-disable-next-line no-use-before-define
+      .catch(abort); // Abort if rawSend returned a rejection.
   };

   /**
@@ -724,7 +729,7 @@ export const makeCapTP = (
         quietReject(obj.reason, false);
         unplug = reason;
         // Deliver the object, even though we're unplugged.
-        rawSend(obj);
+        Promise.resolve(rawSend(obj)).catch(() => {});
       }
       // We no longer wish to subscribe to object finalization.
       slotToImported.clearWithoutFinalizing();