Merge pull request #2528 from murgatroid99/grpc-js_unimplemented_mess…

…age_fix grpc-js: Fix propagation of UNIMPLEMENTED error messages
grpc · Jul 28, 2023 · 15a3f1a · 15a3f1a
2 parents 51d6163 + 4e111e7
commit 15a3f1a
Show file tree

Hide file tree

Showing 3 changed files with 106 additions and 24 deletions.
diff --git a/packages/grpc-js/package.json b/packages/grpc-js/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@grpc/grpc-js",
-  "version": "1.8.20",
+  "version": "1.8.21",
   "description": "gRPC Library for Node - pure JS implementation",
   "homepage": "https://grpc.io/",
   "repository": "https://github.com/grpc/grpc-node/tree/master/packages/grpc-js",

diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts
@@ -61,7 +61,6 @@ import {
 import { parseUri } from './uri-parser';
 import { ChannelzCallTracker, ChannelzChildrenTracker, ChannelzTrace, registerChannelzServer, registerChannelzSocket, ServerInfo, ServerRef, SocketInfo, SocketRef, TlsInfo, unregisterChannelzRef } from './channelz';
 import { CipherNameAndProtocol, TLSSocket } from 'tls';
-import { getErrorCode, getErrorMessage } from './error';
 
 const UNLIMITED_CONNECTION_AGE_MS = ~(1<<31);
 const KEEPALIVE_MAX_TIME_MS = ~(1<<31);
@@ -765,9 +764,7 @@ export class Server {
     return true
   }
 
-  private _retrieveHandler(headers: http2.IncomingHttpHeaders): Handler<any, any> {
-    const path = headers[HTTP2_HEADER_PATH] as string
-
+  private _retrieveHandler(path: string): Handler<any, any> | null {
     this.trace(
       'Received call to method ' +
       path +
@@ -783,7 +780,7 @@ export class Server {
         path +
         '. Sending UNIMPLEMENTED status.'
       );
-      throw getUnimplementedStatusResponse(path);
+      return null;
     }
 
     return handler
@@ -820,15 +817,12 @@ export class Server {
       return
     }
 
-    let handler: Handler<any, any>
-    try {
-      handler = this._retrieveHandler(headers)
-    } catch (err) {
-      this._respondWithError({
-        details: getErrorMessage(err),
-        code: getErrorCode(err) ?? undefined
-      }, stream, channelzSessionInfo)
-      return
+    const path = headers[HTTP2_HEADER_PATH] as string;
+
+    const handler = this._retrieveHandler(path);
+    if (!handler) {
+      this._respondWithError(getUnimplementedStatusResponse(path), stream, channelzSessionInfo);
+      return;
     }
 
     const call = new Http2ServerCallStream(stream, handler, this.options);
@@ -875,15 +869,13 @@ export class Server {
       return
     }
 
-    let handler: Handler<any, any>
-    try {
-      handler = this._retrieveHandler(headers)
-    } catch (err) {
-      this._respondWithError({
-        details: getErrorMessage(err),
-        code: getErrorCode(err) ?? undefined
-      }, stream, null)
-      return
+
+    const path = headers[HTTP2_HEADER_PATH] as string;
+
+    const handler = this._retrieveHandler(path);
+    if (!handler) {
+      this._respondWithError(getUnimplementedStatusResponse(path), stream, null);
+      return;
     }
 
     const call = new Http2ServerCallStream(stream, handler, this.options)

diff --git a/packages/grpc-js/test/test-server.ts b/packages/grpc-js/test/test-server.ts
@@ -408,6 +408,7 @@ describe('Server', () => {
         (error: ServiceError, response: any) => {
           assert(error);
           assert.strictEqual(error.code, grpc.status.UNIMPLEMENTED);
+          assert.match(error.details, /does not implement the method.*Div/);
           done();
         }
       );
@@ -417,6 +418,7 @@ describe('Server', () => {
       const call = client.sum((error: ServiceError, response: any) => {
         assert(error);
         assert.strictEqual(error.code, grpc.status.UNIMPLEMENTED);
+        assert.match(error.details, /does not implement the method.*Sum/);
         done();
       });
 
@@ -433,6 +435,7 @@ describe('Server', () => {
       call.on('error', (err: ServiceError) => {
         assert(err);
         assert.strictEqual(err.code, grpc.status.UNIMPLEMENTED);
+        assert.match(err.details, /does not implement the method.*Fib/);
         done();
       });
     });
@@ -447,6 +450,93 @@ describe('Server', () => {
       call.on('error', (err: ServiceError) => {
         assert(err);
         assert.strictEqual(err.code, grpc.status.UNIMPLEMENTED);
+        assert.match(err.details, /does not implement the method.*DivMany/);
+        done();
+      });
+
+      call.end();
+    });
+  });
+
+  describe('Unregistered service', () => {
+    let server: Server;
+    let client: ServiceClient;
+
+    const mathProtoFile = path.join(__dirname, 'fixtures', 'math.proto');
+    const mathClient = (loadProtoFile(mathProtoFile).math as any).Math;
+
+    before(done => {
+      server = new Server();
+      // Don't register a service at all
+      server.bindAsync(
+        'localhost:0',
+        ServerCredentials.createInsecure(),
+        (err, port) => {
+          assert.ifError(err);
+          client = new mathClient(
+            `localhost:${port}`,
+            grpc.credentials.createInsecure()
+          );
+          server.start();
+          done();
+        }
+      );
+    });
+
+    after(done => {
+      client.close();
+      server.tryShutdown(done);
+    });
+
+    it('should respond to a unary call with UNIMPLEMENTED', done => {
+      client.div(
+        { divisor: 4, dividend: 3 },
+        (error: ServiceError, response: any) => {
+          assert(error);
+          assert.strictEqual(error.code, grpc.status.UNIMPLEMENTED);
+          assert.match(error.details, /does not implement the method.*Div/);
+          done();
+        }
+      );
+    });
+
+    it('should respond to a client stream with UNIMPLEMENTED', done => {
+      const call = client.sum((error: ServiceError, response: any) => {
+        assert(error);
+        assert.strictEqual(error.code, grpc.status.UNIMPLEMENTED);
+        assert.match(error.details, /does not implement the method.*Sum/);
+        done();
+      });
+
+      call.end();
+    });
+
+    it('should respond to a server stream with UNIMPLEMENTED', done => {
+      const call = client.fib({ limit: 5 });
+
+      call.on('data', (value: any) => {
+        assert.fail('No messages expected');
+      });
+
+      call.on('error', (err: ServiceError) => {
+        assert(err);
+        assert.strictEqual(err.code, grpc.status.UNIMPLEMENTED);
+        assert.match(err.details, /does not implement the method.*Fib/);
+        done();
+      });
+    });
+
+    it('should respond to a bidi call with UNIMPLEMENTED', done => {
+      const call = client.divMany();
+
+      call.on('data', (value: any) => {
+        assert.fail('No messages expected');
+      });
+
+      call.on('error', (err: ServiceError) => {
+        assert(err);
+        assert.strictEqual(err.code, grpc.status.UNIMPLEMENTED);
+        assert.match(err.details, /does not implement the method.*DivMany/);
         done();
       });