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.

Sign up for GitHub

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

grpcweb-transport: fix handling responses with empty body and status in headers #331

Merged
merged 1 commit into from
Jul 7, 2022
Merged

grpcweb-transport: fix handling responses with empty body and status in headers #331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/grpcweb-transport/spec/grpc-web-format.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,14 @@ describe('readGrpcWebResponse', () => {
expect(statusInvalid2[0]).toEqual(GrpcStatusCode.INTERNAL);
});

it('handles empty HTTP headers', function () {
const actual1 = readGrpcWebResponseHeader({}, 200, 'success');
expect(actual1).toEqual([undefined, undefined, {}]);

const actual2 = readGrpcWebResponseHeader({}, 400, 'invalid');
expect(actual2).toEqual([GrpcStatusCode.INVALID_ARGUMENT, 'invalid', {}]);
});

it('handles normal Responses', function () {
const headers = new globalThis.Headers([
['foo', 'bar'],
Expand Down Expand Up @@ -249,7 +257,7 @@ describe('readGrpcWebResponse', () => {
const input = [38, 39];
const message = createGrpcWebRequestBody(new Uint8Array(input), 'text');
const [first, ...rest] = asciiToCharCodes(message);

await next([first]);
assertFrames(frames, []);

Expand Down Expand Up @@ -384,7 +392,7 @@ describe('readGrpcWebResponse', () => {
const trailer = readGrpcWebResponseTrailer(new Uint8Array([]));
expect(trailer).toEqual([GrpcStatusCode.OK, undefined, {}])
});

it('handles success status without message', function () {
const trailer = readGrpcWebResponseTrailer(trailerFromObject({
'grpc-status': GrpcStatusCode.OK,
Expand Down
76 changes: 68 additions & 8 deletions packages/grpcweb-transport/spec/grpc-web-transport.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ describe('GrpcWebFetchTransport', () => {
afterAll(() => {
globalThis.fetch = originalFetch;
});

beforeEach(() => {
fetch = jasmine.createSpy('fetch', originalFetch);
globalThis.fetch = fetch;
Expand Down Expand Up @@ -236,7 +236,7 @@ describe('GrpcWebFetchTransport', () => {

// Tailers with OK status, stream finished
await microTaskDelay(next(getTrailerFrame(GrpcStatusCode.OK), true));

// Everything resolved
expect(spy.headers).toHaveBeenCalledTimes(1);
expect(spy.response).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -298,7 +298,39 @@ describe('GrpcWebFetchTransport', () => {
}
});

it('throws `DATA_LOSS` if it does not get a trailer', async () => {
it('throws `INTERNAL` if empty response body and no status in headers', async () => {
const { next, call } = getNextCall();
await next([], true);
try {
await call;
fail('should fail');
} catch (e) {
expect(e).toBeInstanceOf(RpcError);
expect(e.code).toBe(GrpcStatusCode[GrpcStatusCode.INTERNAL]);
}
});

it('throws `DATA_LOSS` if empty response body and OK status in headers', async () => {
const { next, call } = getNextCall({
status: 200,
statusText: 'success',
headers: new globalThis.Headers({
'some': 'header',
'content-type': 'application/grpc-web+proto',
'grpc-status': '0',
}),
});
await next([], true);
try {
await call;
fail('should fail');
} catch (e) {
expect(e).toBeInstanceOf(RpcError);
expect(e.code).toBe(GrpcStatusCode[GrpcStatusCode.DATA_LOSS]);
}
});

it('throws `DATA_LOSS` if it does not get a trailer with response', async () => {
const { next, call } = getNextCall();
await next(getDataFrame('response'), true);
try {
Expand Down Expand Up @@ -383,7 +415,7 @@ describe('GrpcWebFetchTransport', () => {
expect(e.code).toBe(GrpcStatusCode[GrpcStatusCode.INTERNAL]);
}
});

});

describe('serverStreaming()', () => {
Expand Down Expand Up @@ -421,7 +453,7 @@ describe('GrpcWebFetchTransport', () => {
afterAll(() => {
globalThis.fetch = originalFetch;
});

beforeEach(() => {
fetch = jasmine.createSpy('fetch', originalFetch);
globalThis.fetch = fetch;
Expand Down Expand Up @@ -514,7 +546,7 @@ describe('GrpcWebFetchTransport', () => {

// Tailers with OK status, stream finished
await microTaskDelay(next(getTrailerFrame(GrpcStatusCode.OK), true));

// Everything resolved
expect(spy.headers).toHaveBeenCalledTimes(1);
expect(responsesIteratorSpy).toHaveBeenCalledTimes(3);
Expand Down Expand Up @@ -578,7 +610,35 @@ describe('GrpcWebFetchTransport', () => {
}
});

it('throws `DATA_LOSS` if it does not get a trailer', async () => {
it('throws `INTERNAL` if empty response body and no status in headers', async () => {
const { next, call } = getNextCall();
await next([], true);
try {
await call;
fail('should fail');
} catch (e) {
expect(e).toBeInstanceOf(RpcError);
expect(e.code).toBe(GrpcStatusCode[GrpcStatusCode.INTERNAL]);
}
});

it('success if empty response body and OK status in headers', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses

Most responses are expected to have both headers and trailers but Trailers-Only is permitted for calls that produce an immediate error.

Thus I would expect any "Trailers-Only" response to throw an RpcError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is of course logical for non OK status codes. But in practice, it will be strange if received code OK will be accepted as error on client side.
Maybe initially this case (sending from server only status OK with empty body in stream) is strange, because I think normally server should send something like NOT_FOUND code if there is nothing to send in stream.
But at the moment exactly this handling is implemented in the official implementation of the protocol in https://github.com/grpc/grpc-web

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sentence in the spec is frustratingly ambiguous. And unfortunately none of the official implementation tests actually assert what should happen when getting an OK trailers-only unary RPC response: https://github.com/grpc/grpc-web/blob/master/packages/grpc-web/test/generated_code_test.js

Though searching through issues on grpc/grpc I get the sense that it is not allowed for the server to send an OK status without a message in a unary RPC. From grpc/grpc-java#2785 (comment):

Unary must either error or have a message.

The server does not enforce "unary must have one message." If the application closes with OK but no messages, this should become an error of some sort (sent to the client). This would ideally be done in ServerCallImpl.

From grpc/grpc#8010 (comment):

Is the server calling StreamObserver.onCompleted() (or ServerCall.close(Status.OK,...)) without first sending a message?

[...] no problem. It made us realize we weren't verifying that on server-side.

From grpc/grpc-java#2243:

It appears that a server can respond OK to a unary RPC without any detection on the server-side. It'd be nice to throw an exception if close(OK) is called without first sending a message for a UNARY response, although that may be against our API stability. At the very least, we should convert the OK into a Status saying the server is broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For unary RPC, I have not changed the final behavior and in this case there will be DATA_LOSS error code. See test at line 313.

In general, while there is no unambiguous interpretation yet of this case with streams, it is better to have an implementation compatible with other libraries so that it is easier for developers to migrate from one implementation to another.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, my mistake. This makes sense for serverStreaming.

const { next, call } = getNextCall({
status: 200,
statusText: 'success',
headers: new globalThis.Headers({
'some': 'header',
'content-type': 'application/grpc-web+proto',
'grpc-status': '0',
}),
});
await next([], true);
const streamResult = await call;
expect(streamResult.status.code).toBe('OK');
expect(streamResult.headers).toEqual({ some: 'header' });
});

it('throws `DATA_LOSS` if it does not get a trailer with response', async () => {
const { next, call } = getNextCall();
await next(getDataFrame('response'), true);
try {
Expand Down Expand Up @@ -642,7 +702,7 @@ describe('GrpcWebFetchTransport', () => {
expect(e.code).toBe(GrpcStatusCode[GrpcStatusCode.INTERNAL]);
}
});

});


Expand Down
16 changes: 8 additions & 8 deletions packages/grpcweb-transport/src/grpc-web-format.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ export function createGrpcWebRequestBody(message: Uint8Array, format: GrpcWebFor
* If given a fetch response, checks for fetch-specific error information
* ("type" property) and whether the "body" is null and throws a RpcError.
*/
export function readGrpcWebResponseHeader(fetchResponse: Response): [GrpcStatusCode, string | undefined, RpcMetadata];
export function readGrpcWebResponseHeader(headers: HttpHeaders, httpStatus: number, httpStatusText: string): [GrpcStatusCode, string | undefined, RpcMetadata];
export function readGrpcWebResponseHeader(headersOrFetchResponse: HttpHeaders | Response, httpStatus?: number, httpStatusText?: string): [GrpcStatusCode, string | undefined, RpcMetadata] {
export function readGrpcWebResponseHeader(fetchResponse: Response): [GrpcStatusCode | undefined, string | undefined, RpcMetadata];
export function readGrpcWebResponseHeader(headers: HttpHeaders, httpStatus: number, httpStatusText: string): [GrpcStatusCode | undefined, string | undefined, RpcMetadata];
export function readGrpcWebResponseHeader(headersOrFetchResponse: HttpHeaders | Response, httpStatus?: number, httpStatusText?: string): [GrpcStatusCode | undefined, string | undefined, RpcMetadata] {
if (arguments.length === 1) {
let fetchResponse = headersOrFetchResponse as Response;

Expand Down Expand Up @@ -113,7 +113,7 @@ export function readGrpcWebResponseHeader(headersOrFetchResponse: HttpHeaders |
responseMeta = parseMetadata(headers),
[statusCode, statusDetail] = parseStatus(headers);

if (statusCode === GrpcStatusCode.OK && !httpOk) {
if ((statusCode === undefined || statusCode === GrpcStatusCode.OK) && !httpOk) {
statusCode = httpStatusToGrpc(httpStatus!);
statusDetail = httpStatusText;
}
Expand All @@ -134,7 +134,7 @@ export function readGrpcWebResponseTrailer(data: Uint8Array): [GrpcStatusCode, s
headers = parseTrailer(data),
[code, detail] = parseStatus(headers),
meta = parseMetadata(headers);
return [code, detail, meta];
return [code ?? GrpcStatusCode.OK, detail, meta];
}


Expand Down Expand Up @@ -310,9 +310,9 @@ function parseFormat(contentType: string | undefined | null): GrpcWebFormat {
}


// returns error code on parse failure, uses OK as default code
function parseStatus(headers: HttpHeaders): [GrpcStatusCode, string | undefined] {
let code = GrpcStatusCode.OK,
// returns error code on parse failure
function parseStatus(headers: HttpHeaders): [GrpcStatusCode | undefined, string | undefined] {
let code: GrpcStatusCode | undefined,
message: string | undefined;
let m = headers['grpc-message'];
if (m !== undefined) {
Expand Down
25 changes: 18 additions & 7 deletions packages/grpcweb-transport/src/grpc-web-transport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ export class GrpcWebFetchTransport implements RpcTransport {
inputBytes = method.I.toBinary(input, opt.binaryOptions),
defHeader = new Deferred<RpcMetadata>(),
responseStream = new RpcOutputStreamController<O>(),
responseEmptyBody = true,
maybeStatus: RpcStatus | undefined,
defStatus = new Deferred<RpcStatus>(),
maybeTrailer: RpcMetadata | undefined,
Expand All @@ -109,8 +110,13 @@ export class GrpcWebFetchTransport implements RpcTransport {
.then(fetchResponse => {
let [code, detail, meta] = readGrpcWebResponseHeader(fetchResponse);
defHeader.resolve(meta);
if (code !== GrpcStatusCode.OK)
if (code != null && code !== GrpcStatusCode.OK)
throw new RpcError(detail ?? GrpcStatusCode[code], GrpcStatusCode[code], meta);
if (code != null)
maybeStatus = {
code: GrpcStatusCode[code],
detail: detail ?? GrpcStatusCode[code]
};
return fetchResponse;
})

Expand All @@ -123,6 +129,7 @@ export class GrpcWebFetchTransport implements RpcTransport {
responseStream.notifyMessage(
method.O.fromBinary(data, opt.binaryOptions)
);
responseEmptyBody = false;
break;
case GrpcWebFrame.TRAILER:
let code, detail;
Expand All @@ -137,8 +144,7 @@ export class GrpcWebFetchTransport implements RpcTransport {
})

.then(() => {

if (!maybeTrailer)
if (!maybeTrailer && !responseEmptyBody)
throw new RpcError(`missing trailers`, GrpcStatusCode[GrpcStatusCode.DATA_LOSS]);

// istanbul ignore if - this should be impossible and only here to satisfy TypeScript
Expand All @@ -150,7 +156,7 @@ export class GrpcWebFetchTransport implements RpcTransport {

responseStream.notifyComplete();
defStatus.resolve(maybeStatus);
defTrailer.resolve(maybeTrailer);
defTrailer.resolve(maybeTrailer || {});

})

Expand Down Expand Up @@ -212,8 +218,13 @@ export class GrpcWebFetchTransport implements RpcTransport {
.then(fetchResponse => {
let [code, detail, meta] = readGrpcWebResponseHeader(fetchResponse);
defHeader.resolve(meta);
if (code !== GrpcStatusCode.OK)
if (code != null && code !== GrpcStatusCode.OK)
throw new RpcError(detail ?? GrpcStatusCode[code], GrpcStatusCode[code], meta);
if (code != null)
maybeStatus = {
code: GrpcStatusCode[code],
detail: detail ?? GrpcStatusCode[code]
};
return fetchResponse;
})

Expand All @@ -240,7 +251,7 @@ export class GrpcWebFetchTransport implements RpcTransport {
})

.then(() => {
if (!maybeTrailer)
if (!maybeTrailer && maybeMessage)
throw new RpcError(`missing trailers`, GrpcStatusCode[GrpcStatusCode.DATA_LOSS]);

// istanbul ignore if - this should be impossible and only here to satisfy TypeScript
Expand All @@ -258,7 +269,7 @@ export class GrpcWebFetchTransport implements RpcTransport {
throw new RpcError(maybeStatus.detail, maybeStatus.code, maybeTrailer);

defStatus.resolve(maybeStatus);
defTrailer.resolve(maybeTrailer);
defTrailer.resolve(maybeTrailer || {});

})

Expand Down