Grpc-web: Unary callback executed multiple times on gRPC error

[ascherkus]

I notice on HTTP/1.1 200 responses with a valid gRPC error that the callback is being executed three times. I believe it's a result of having a non-OK status code, which leads to executing the callback twice on "status" events and once more on the "error" event in grpcwebclientbase.js.

I found this to be surprising behavior (I expected at most a single non-null error callback), and since the callback is a required parameter I either have to use a local variable to track whether a non-null error has been previously encountered to prevent triggering error logic multiple times _or_ pass in no-op callback and use the returned ClientReadableStream object.

Example server code (Go):

func (s *server) Foo(context.Context, *foo.FooRequest) (*foo.FooResponse, error) {
    return nil, status.Error(codes.Unimplemented, "unimplemented")
}

Example client code (Typescript):

let client = new FooClient("http://localhost:8888");
let req = new FooRequest();
client.foo(req, null, function(err, resp) {
  console.log(err);
  console.log(resp);
  console.trace();
});

Console output:

{code: 12, message: "unimplemented"}
null
[ stack trace where "status" callback is triggered ]
{code: 12, message: "unimplemented"}
null
[ stack trace where "status" callback is triggered ]
{code: 12, message: "unimplemented"}
null
[ stack trace where "error" callback is triggered ]

I'm curious whether this is intended behavior or not (the double-status event in particular seems odd).

If not, I've got a few ideas:

1. Make the callback an optional parameter
2. Remove the callback entirely to support/force a consistent coding approach (i.e., same as streaming)
3. Don't execute the callback for status updates (i.e., only execute on "data" or "error")
4. Call the callback at most once on the first non-OK status/error response (whichever comes first)

Environment details:

• protoc-gen-grpc-web-1.0.6-darwin-x86_64
• protoc 3.7.1
• --js_out=import_style=commonjs:$OUT
• --grpc-web_out=import_style=typescript,mode=grpcweb:$OUT
• "google-protobuf": "^3.10.0-rc.1"
• "grpc-web": "^1.0.6"
• Server using github.com/improbable-eng/grpc-web/go/grpcweb

[stanley-cheung]

This is probably fixed in #608, that hasn't been released yet. Will be included in the next release, namely 1.0.7.

[ascherkus]

I tested locally against HEAD (192fef8909dcbc0c9889c348ec092ecf657c62e8) and while the behavior has changed, I still feel it's confusing as an end user.

Sample code:

let stream = client.foo(req, null, function() {});
stream.on("status", function(err) {
  console.log("status")
});
stream.on("error", function(err) {
  console.log("error")
});
stream.on("data", function(resp) {
  console.log("data")
});

1.0.6 behavior:

status
error

HEAD behavior:

status

We now get one less status callback and the order of error and status are swapped. Using a callback still results in it firing twice on errors as opposed to three times.

I also noticed that mixing/matching stream.on() with the callback results in "hijacking" the events and prevents the callback being fired (e.g., registering stream.on("status", ...) will mean the callback will not get called on status updates). This was also surprising to me.

While I'm unblocked for my own project (I pass in no-op callbacks and exclusively use stream.on("error", ...) and stream.on("data", ...)), this seems less than ideal for newcomers to grpc-web.

Do you have an opinion on whether this is by design? Happy to contribute code, tests, documentation to clarify!

[vsile]

On 01 May 2020 I steel have the issue.

[AdamiHamza]

Still having this issue 😞

[stanley-cheung]

This is probably fixed in 1.1.0. If this is still an issue, please re-open.

[ascherkus]

Thanks for the fix!! I'll give it whirl!

[stanley-cheung]

Just to clarify a few things, related to this particular issue:

• 847 should fix the "calling .on() will _override_ the existing callback" problem. Now you should be able to add multiple callbacks to the same event, and they will each be called in sequence. There is also a new removeListener() method to remove a callback you added.

• This should also fix the problem where either the error or the status callback is being called multiple times per RPC. If there is an error, the unary callback (err, response) => {} should only be called once now, with err being populated. Same for the status callback, in itself, should only be called once.

• Now, with regard to status and error both being called on an error, I think this might still be an issue. I am looking into how grpc-node handles this, and looks like there's a matrix of cases here:

  • unary vs streaming
  • normal completion of RPC with trailing status vs actual errors (e.g. unimplemented, deadline exceeded, network error)
  • .on('status') vs .on('error')