Troubleshoot and fix issues in grpc-web JS conformance client (#831)

There were a couple of bugs in the conformance client that were
causing some tests to outright hang (promise was never resolved)
and some cases to have an incorrect result sent back to the test
runner (causing some tests that were actually correct/passing to
be misinterpreted as failing).

This adds some logging to the client so that any uncaught errors
in the browser can be logged to stderr (instead of being trapped
in the headless browser's JS console). It also adds a safeguard to
prevent hanging in the future: if a test takes longer than 15s, the
client will mark it as a failure and stop waiting for the promise to
be resolved.

Makefile

@@ -110,11 +110,7 @@ runclienttests: $(BIN)/connectconformance $(BIN)/referenceclient $(BIN)/grpcclie
 	$(BIN)/connectconformance -v --conf ./testing/grpc-impls-config.yaml --mode client --trace \
 		--known-failing @./testing/grpcclient-known-failing.txt \
 		-- $(BIN)/grpcclient
-	@# Note that use of --skip is discouraged, but if we don't skip them the client crashes.
-	@# TODO: troubleshoot the skipped test cases and figure out why they crash.
 	$(BIN)/connectconformance -v --conf ./testing/grpc-web-client-impl-config.yaml --mode client --trace \
-		--skip "**/trailers-only/missing-status" \
-		--skip "**/trailers-only/unary-ok-but-no-response" \
 		--known-failing @./testing/grpcwebclient-known-failing.txt \
 		-- ./testing/grpcwebclient/bin/grpcwebclient
 

internal/app/connectconformance/server_runner.go

@@ -206,8 +206,10 @@ func runTestCasesForServer(
 				results.setOutcome(name, true, err)
 			case resp.GetError() != nil:
 				results.failed(name, resp.GetError())
-			default:
+			case resp.GetResponse() != nil:
 				results.assert(name, testCase, resp.GetResponse())
+			default:
+				results.setOutcome(name, false, errors.New("client returned a response with neither an error nor result"))
 			}
 			if isReferenceClient && resp.GetResponse() != nil {
 				for _, msg := range resp.GetResponse().Feedback {

testing/grpcwebclient-known-failing.txt

@@ -16,11 +16,6 @@ Duplicate Metadata/**/server-stream/**
 gRPC-Web Trailers/**/trailers-in-body/duplicate-metadata
 gRPC-Web Trailers/**/trailers-in-body/mixed-case
 
-# The gRPC-Web client also fails to verify the grpc-status. If it is missing, it assumes the
-# result is okay, even for a unary RPC where there were zero response messages.
-gRPC-Web Unexpected Responses/**/missing-status
-gRPC-Web Unexpected Responses/**/unary-ok-but-no-response
-
 # The gRPC-Web client also does not use the expected error codes for some error situations.
 # It appears to use "unknown" for nearly (which likely means it's just not classifying them
 # at all, which is certainly a bug).

testing/grpcwebclient/browserscript.ts

@@ -36,15 +36,36 @@ import {
 } from "grpc-web";
 
 // The main entry point into the browser code running in Puppeteer/headless Chrome.
-// This function is invoked by the page.evalulate call in grpcwebclient.
+// This function is invoked by the page.evaluate call in grpcwebclient.
 async function runTestCase(data: number[]): Promise<number[]> {
   const request = ClientCompatRequest.deserializeBinary(new Uint8Array(data));
 
-  const result = await invoke(request);
+  const rpcResult = invoke(request);
+  const timeout = new Promise<ClientResponseResult>((_, reject) => {
+    // Fail if we still don't have a response after 15 seconds
+    // so that user can at least see exactly which test case timed out.
+    setTimeout(() => {
+      reject(new Error("promise never resolved after 15s!"));
+    }, 15*1000);
+  });
+  const result = await Promise.race([rpcResult, timeout]);
 
   return Array.from(result.serializeBinary());
 }
 
+function addErrorListeners() {
+  window.addEventListener("error", function (e) {
+    // @ts-ignore
+    window.log("ERROR: uncaught error in browser: " + e.error.filename + ":" + e.error.lineno + ": " + e.message);
+    return false;
+  })
+  window.addEventListener("unhandledrejection", function (e) {
+    // @ts-ignore
+    window.log("ERROR: unhandled promise failure in browser: " + e.reason);
+    return false;
+  })
+}
+
 function invoke(req: ClientCompatRequest) {
   const client = createClient(req);
   switch (req.getMethod()) {
@@ -117,10 +138,13 @@ function convertGrpcToProtoError(rpcErr: RpcError): ProtoError {
   err.setCode(convertStatusCodeToCode(rpcErr.code));
   err.setMessage(rpcErr.message);
 
-  const value = rpcErr.metadata["grpc-status-details-bin"];
-  if (value) {
-    const status = Status.deserializeBinary(stringToUint8Array(atob(value)));
-    err.setDetailsList(status.getDetailsList());
+  let md = rpcErr.metadata;
+  if (md !== undefined) {
+    const value = md["grpc-status-details-bin"];
+    if (value) {
+      const status = Status.deserializeBinary(stringToUint8Array(atob(value)));
+      err.setDetailsList(status.getDetailsList());
+    }
   }
 
   return err;
@@ -194,6 +218,15 @@ async function unary(
     (err: RpcError, response: UnaryResponse) => {
       if (err !== null) {
         resp.setError(convertGrpcToProtoError(err));
+        let md = err.metadata;
+        if (md !== undefined) {
+          resp.setResponseTrailersList(convertMetadataToHeader(md));
+        }
+        // Ideally, we'd complete the promise from the "end" event. However,
+        // most RPCs that result in an RPC error (as of 3/15/2024, 50 out of
+        // 57 failed RPCs) do not produce an "end" event after the callback
+        // is invoked with an error.
+        res(resp);
       } else {
         const payload = response.getPayload();
         if (payload !== undefined) {
@@ -212,13 +245,22 @@ async function unary(
 
   // Response trailers (i.e. trailing metadata) are sent in the 'status' event
   result.on("status", (status: GrpcWebStatus) => {
+    // One might expect that the "status" event is always delivered (since
+    // consistency would make it much easier to implement interceptors or
+    // decorators, to instrument all RPCs with cross-cutting concerns, like
+    // metrics, logging, etc). But one would be wrong: as of 3/15/2024, there
+    // are 2 cases where the "status" event is never delivered (both cases
+    // are RPC failures).
     const md = status.metadata;
     if (md !== undefined) {
       resp.setResponseTrailersList(convertMetadataToHeader(md));
-      res(resp);
     }
   });
 
+  result.on("end", () => {
+    res(resp);
+  });
+
   return prom;
 }
 
@@ -266,18 +308,31 @@ async function serverStream(
   });
   stream.on("error", (err: RpcError) => {
     resp.setError(convertGrpcToProtoError(err));
+    let md = err.metadata;
+    if (md !== undefined) {
+      resp.setResponseTrailersList(convertMetadataToHeader(md));
+    }
+    // Ideally, we'd complete the promise from the "end" event. However, there
+    // are some RPCs that result in an RPC error (as of 3/15/2024, 3 out of 44
+    // failed RPCs) that do not produce an "end" event after the "error" event.
     res(resp);
   });
 
   // Response trailers (i.e. trailing metadata) are sent in the 'status' event
   stream.on("status", (status: GrpcWebStatus) => {
+    // One might expect that the "status" event is always delivered (since
+    // consistency would make it much easier to implement interceptors or
+    // decorators, to instrument all RPCs with cross-cutting concerns, like
+    // metrics, logging, etc). But one would be wrong: as of 3/15/2024, there
+    // is one case (out of 62 total RPCs) where the "status" event is never
+    // delivered for a streaming call.
     const md = status.metadata;
     if (md !== undefined) {
       resp.setResponseTrailersList(convertMetadataToHeader(md));
     }
   });
 
-  stream.on("end", function () {
+  stream.on("end", () => {
     res(resp);
   });
 
@@ -337,3 +392,5 @@ async function unimplemented(
 
 // @ts-ignore
 window.runTestCase = runTestCase;
+// @ts-ignore
+window.addErrorListeners = addErrorListeners;

testing/grpcwebclient/grpcwebclient.ts

@@ -23,15 +23,31 @@ import {
 } from "./gen/proto/connectrpc/conformance/v1/client_compat_pb.js";
 
 export async function run() {
-  // Launch a browser. For a non-headless browser, pass false
+  // Launch a browser. For a non-headless browser, pass {headless: false}.
+  // Note that the test runner kills the process at the end if it takes too
+  // long to shut down, and puppeteer can't/won't leave browser open after
+  // this script terminates, so the value of non-headless browser is quite
+  // limited. It may be more effective for troubleshooting to instead add
+  // calls to window.log in browserscript.ts (these messages will show up
+  // in the test runner output).
   const browser = await puppeteer.launch({ headless: "new" });
   const page = await browser.newPage();
 
+  await page.exposeFunction("log", (message: string) => {
+    process.stderr.write(message + "\n");
+  })
   await page.addScriptTag({
     type: "application/javascript",
     content: await buildBrowserScript(),
   });
 
+  await page.evaluate(() => {
+    // This allows us log uncaught errors in the browser to this script's
+    // stderr, so they can be seen in the console output when running tests.
+    // @ts-ignore
+    return window.addErrorListeners();
+  });
+
   for await (const next of readReqBuffers(process.stdin)) {
     const req = ClientCompatRequest.deserializeBinary(next);
     const res = new ClientCompatResponse();