Grpc-web: gRPC status code header destroyed on error

[hectim]

Been using this grpc-web client on a project for a while, but recently started handling specific grpc errors on the front-end. Our requests hit an Envoy proxy, which does its own httpToGrpc status conversion, but in the grpc-web-client code specifically we are also losing the grpc-status header.

Through some local testing, it looks like the problem comes from https://github.com/improbable-eng/grpc-web/blob/master/ts/src/client.ts#L99

const code = httpStatusToCode(status);
this.props.debug && debug("onHeaders.code", code);
const gRPCMessage = headers.get("grpc-message") || [];
this.props.debug && debug("onHeaders.gRPCMessage", gRPCMessage);
if (code !== Code.OK) {
  const statusMessage = this.decodeGRPCStatus(gRPCMessage[0]);
  this.rawOnError(code, statusMessage);
  return;
}

My problem could be solved if instead of passing the newly created code to rawOnError, it checked for a grpc-status header first and used that value before defaulting to the new code. Something like:

const code = parseInt(headers.get("grpc-status"), 10) || httpStatusToCode(status);

This seems to be the simplest way to fix it, although there may be reasons why this is unwanted.

Option 2: If we made sure the headers were being passed correctly, we could just pull them off the response. Right now when it throws an error it makes a new Metadata() and hands that off, but it that isn't very helpful.

rawOnError(code: Code, msg: string) {
    this.props.debug && debug("rawOnError", code, msg);
    if (this.completed) return;
    this.completed = true;
    this.onEndCallbacks.forEach(callback => {
      detach(() => {
        callback(code, msg, new Metadata());
      });
    });
  }

If instead, rawOnError took an optional third parameter of the headers

rawOnError(code: Code, msg: string, headers?: Metadata) {

and passed those (if they exist) to the callback, we could accomplish essentially the same thing.

callback(code, msg, headers || new Metadata());

I could submit a PR for this anytime, just wanted to know if there were any extra thoughts about potential hiccups or problems with either of these. Personally, I would opt for the first option for simplicity.

[easyCZ]

Hi @hectim,

Sounds like a good use case and your solution seems perfectly reasonable. I'd be happy to look at a PR with the above change. Thanks for raising!

[gunn4r]

@easyCZ I'm working with @hectim on this and will be putting together the actual PR. Thanks a bunch!

[gunn4r]

@easyCZ After digging through this more, it appears the tests that are testing "error" cases are not actually ever hitting the error case here:

      const code = httpStatusToCode(status);

      this.props.debug && debug("onHeaders.code", code);

      const gRPCMessage = headers.get("grpc-message") || [];

      this.props.debug && debug("onHeaders.gRPCMessage", gRPCMessage);

      if (code !== Code.OK) {
        const statusMessage = this.decodeGRPCStatus(gRPCMessage[0]);
        this.rawOnError(code, statusMessage);
        return;
      }

      this.rawOnHeaders(headers);

(This is in client.ts)

The test server returns 200 responses even when "testing" the grpc error statuses, and with the current way the responses are handled that gets converted into a grpc status of 0 by the httpStatusToCode method.

So in the tests, while the test server returns a grpc error status, the http status is 200, thus bypassing the if statement. Many tests test for "headers / trailers" which are never available inside the error case. So it looks like to me that the tests are giving false positives.

I still think the appropriate change is to NOT do the http code conversion if there is a valid grpc-status on the headers. However this would mean that for users who are currently returning http 200 but with a grpc-error code, they would now be hitting the error case and I can see this being a breaking change. That said, I still think its an appropriate change.

Are you guys willing to address this issue? What are your thoughts?

---

Here is an example of a test that is exhibiting this behavior:
(https://github.com/improbable-eng/grpc-web/blob/master/test/ts/src/invoke.spec.ts#L283)

headerTrailerCombos((withHeaders, withTrailers) => {
        it("should report status code for error with headers + trailers", (done) => {
          let didGetOnHeaders = false;
          let didGetOnMessage = false;

          const ping = new PingRequest();
          ping.setFailureType(PingRequest.FailureType.CODE);
          ping.setErrorCodeReturned(12);
          ping.setSendHeaders(withHeaders);
          ping.setSendTrailers(withTrailers);

          grpc.invoke(TestService.PingError, {
            debug: DEBUG,
            transport: transport,
            request: ping,
            host: testHostUrl,
            onHeaders: (headers: grpc.Metadata) => {
              DEBUG && debug("headers", headers);
              didGetOnHeaders = true;
            },
            onMessage: (message: Empty) => {
              didGetOnMessage = true;
            },
            onEnd: (status: grpc.Code, statusMessage: string, trailers: grpc.Metadata) => {
              DEBUG && debug("status", status, "statusMessage", statusMessage, "trailers", trailers);
              assert.deepEqual(trailers.get("grpc-status"), ["12"]);
              assert.deepEqual(trailers.get("grpc-message"), ["Intentionally returning error for PingError"]);
              assert.strictEqual(status, grpc.Code.Unimplemented);
              assert.strictEqual(statusMessage, "Intentionally returning error for PingError");
              assert.ok(didGetOnHeaders);
              assert.ok(!didGetOnMessage);
              done();
            }
          });
        });
      });

This test is checking for headers/trailers with a grpc error code. It works currently because the server while it does add the grpc-status 12 to the response, the http status is 200. It never hits the rawOnError inside the if (code !== Code.OK) { block, and instead always hits the this.rawOnHeaders(headers); just after that check. The primary difference being that the headers are provided to rawOnHeaders but not to rawOnError.

When I make the proposed change it causes many of the tests to fail because they are no longer hitting the rawOnHeaders and instead hitting the rawOnError.

[gunn4r]

If I make the original proposed changes, and also call rawOnHeaders when there is a grpc error code then every thing works peachy and all the tests pass. Basically move the this.rawOnHeaders(headers); from below the if statement checking the grpc code to above it. I'm not seeing any complications from that but I could be wrong. It WOULD mean that onHeaders callbacks would get called on errors as well as non errors but with how you guys are currently using it that seems to be the case anyway (since you guys are returning http 200s all the time regardless of the grpc status). Let me know what you think @easyCZ.

This is what that change looks like:

onTransportHeaders(headers: Metadata, status: number) {
    this.props.debug && debug("onHeaders", headers, status);

    if (this.closed) {
      this.props.debug && debug("grpc.onHeaders received after request was closed - ignoring");
      return;
    }

    if (status === 0) {
      // The request has failed due to connectivity issues. Do not capture the headers
    } else {
      this.responseHeaders = headers;
      this.props.debug && debug("onHeaders.responseHeaders", JSON.stringify(this.responseHeaders, null, 2));

      const gRPCStatus: Code = parseInt(headers.get("grpc-status")[0], 10);

      const code: Code = gRPCStatus >= 0 ? gRPCStatus : httpStatusToCode(status);

      this.props.debug && debug("onHeaders.code", code);

      const gRPCMessage = headers.get("grpc-message") || [];

      this.props.debug && debug("onHeaders.gRPCMessage", gRPCMessage);

      this.rawOnHeaders(headers);

      if (code !== Code.OK) {
        const statusMessage = this.decodeGRPCStatus(gRPCMessage[0]);
        this.rawOnError(code, statusMessage, headers);
      }
    }
  }

  rawOnError(code: Code, msg: string, trailers?: Metadata) {
    this.props.debug && debug("rawOnError", code, msg);
    if (this.completed) return;
    this.completed = true;
    this.onEndCallbacks.forEach(callback => {
      detach(() => {
        callback(code, msg, trailers || new Metadata());
      });
    });
  }

[jonny-improbable]

I believe this was fixed by #226.