Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors thrown in grpc method are ignored #7

Open
n0v1 opened this issue Oct 14, 2020 · 1 comment
Open

Errors thrown in grpc method are ignored #7

n0v1 opened this issue Oct 14, 2020 · 1 comment

Comments

@n0v1
Copy link

n0v1 commented Oct 14, 2020

When an error is thrown in a grpc method, the grpc server will normally crash with UNCAUGHT EXCEPTION Error. When using ges, the error is ignored, the server keeps running and the call never finishes because the grpc callback isn't executed.

Example:

service.proto:

syntax = "proto3";

package dev.test;

import "google/protobuf/empty.proto";

service MyService {
  rpc test(google.protobuf.Empty) returns (google.protobuf.Empty);
}

server.js:

[...]
const server = new ExperimentalServer();

const test = (call, callback) => {
  throw new Error('Some Error');
}
server.addService(serviceDefinition, {test: test});

server.use(async (context, next) => {
  await next()
})

This happens because in the catch clause the variable handled will be true in this case (the grpc method was already called). Then the error will just be logged but the callback never executed.

I'd suggest to either never ignore errors or make sure that handled is set to true only after the original implementation was executed without throwing an error.

@edvardchen
Copy link
Owner

I knew it. I used to make this experimental feature transparent. They don't need to know its existence. Their method handlers should behave the same in both ExperimentalServer and the original grpc.Server.

When you throw error in method handler, the original grpc.Server would just make the process exit.

Now, I see the code. Since we wrap the method handler in async function, then we would throw unhandled rejection warning instead of exiting.

So I think your suggestion is reasonable. We can send the error to the client

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants