From 374309be66b908a58de667587d6eaa87cef3de93 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 5 Jan 2021 09:15:17 -0800 Subject: [PATCH] grpc-js: Propagate internal stream errors from the http2 module --- packages/grpc-js/src/call-stream.ts | 138 +++++++++++++++++----------- 1 file changed, 82 insertions(+), 56 deletions(-) diff --git a/packages/grpc-js/src/call-stream.ts b/packages/grpc-js/src/call-stream.ts index 91e24ea55..9c27aeb1f 100644 --- a/packages/grpc-js/src/call-stream.ts +++ b/packages/grpc-js/src/call-stream.ts @@ -37,6 +37,10 @@ const { NGHTTP2_CANCEL, } = http2.constants; +interface NodeError extends Error { + code: string; +} + export type Deadline = Date | number; export interface CallStreamOptions { @@ -202,6 +206,8 @@ export class Http2CallStream implements Call { private listener: InterceptingListener | null = null; + private internalErrorMessage: string | null = null; + constructor( private readonly methodName: string, private readonly channel: ChannelImplementation, @@ -518,66 +524,86 @@ export class Http2CallStream implements Call { this.maybeOutputStatus(); }); stream.on('close', () => { - this.trace('HTTP/2 stream closed with code ' + stream.rstCode); - /* If we have a final status with an OK status code, that means that - * we have received all of the messages and we have processed the - * trailers and the call completed successfully, so it doesn't matter - * how the stream ends after that */ - if (this.finalStatus?.code === Status.OK) { - return; - } - let code: Status; - let details = ''; - switch (stream.rstCode) { - case http2.constants.NGHTTP2_NO_ERROR: - /* If we get a NO_ERROR code and we already have a status, the - * stream completed properly and we just haven't fully processed - * it yet */ - if (this.finalStatus !== null) { - return; - } - code = Status.INTERNAL; - details = `Received RST_STREAM with code ${stream.rstCode}`; - break; - case http2.constants.NGHTTP2_REFUSED_STREAM: - code = Status.UNAVAILABLE; - details = 'Stream refused by server'; - break; - case http2.constants.NGHTTP2_CANCEL: - code = Status.CANCELLED; - details = 'Call cancelled'; - break; - case http2.constants.NGHTTP2_ENHANCE_YOUR_CALM: - code = Status.RESOURCE_EXHAUSTED; - details = 'Bandwidth exhausted'; - break; - case http2.constants.NGHTTP2_INADEQUATE_SECURITY: - code = Status.PERMISSION_DENIED; - details = 'Protocol not secure enough'; - break; - case http2.constants.NGHTTP2_INTERNAL_ERROR: - code = Status.INTERNAL; - /* This error code was previously handled in the default case, and - * there are several instances of it online, so I wanted to - * preserve the original error message so that people find existing - * information in searches, but also include the more recognizable - * "Internal server error" message. */ - details = `Received RST_STREAM with code ${stream.rstCode} (Internal server error)`; - break; - default: - code = Status.INTERNAL; - details = `Received RST_STREAM with code ${stream.rstCode}`; - } - // This is a no-op if trailers were received at all. - // This is OK, because status codes emitted here correspond to more - // catastrophic issues that prevent us from receiving trailers in the - // first place. - this.endCall({ code, details, metadata: new Metadata() }); + /* Use process.next tick to ensure that this code happens after any + * "error" event that may be emitted at about the same time, so that + * we can bubble up the error message from that event. */ + process.nextTick(() => { + this.trace('HTTP/2 stream closed with code ' + stream.rstCode); + /* If we have a final status with an OK status code, that means that + * we have received all of the messages and we have processed the + * trailers and the call completed successfully, so it doesn't matter + * how the stream ends after that */ + if (this.finalStatus?.code === Status.OK) { + return; + } + let code: Status; + let details = ''; + switch (stream.rstCode) { + case http2.constants.NGHTTP2_NO_ERROR: + /* If we get a NO_ERROR code and we already have a status, the + * stream completed properly and we just haven't fully processed + * it yet */ + if (this.finalStatus !== null) { + return; + } + code = Status.INTERNAL; + details = `Received RST_STREAM with code ${stream.rstCode}`; + break; + case http2.constants.NGHTTP2_REFUSED_STREAM: + code = Status.UNAVAILABLE; + details = 'Stream refused by server'; + break; + case http2.constants.NGHTTP2_CANCEL: + code = Status.CANCELLED; + details = 'Call cancelled'; + break; + case http2.constants.NGHTTP2_ENHANCE_YOUR_CALM: + code = Status.RESOURCE_EXHAUSTED; + details = 'Bandwidth exhausted'; + break; + case http2.constants.NGHTTP2_INADEQUATE_SECURITY: + code = Status.PERMISSION_DENIED; + details = 'Protocol not secure enough'; + break; + case http2.constants.NGHTTP2_INTERNAL_ERROR: + code = Status.INTERNAL; + if (this.internalErrorMessage === null) { + /* This error code was previously handled in the default case, and + * there are several instances of it online, so I wanted to + * preserve the original error message so that people find existing + * information in searches, but also include the more recognizable + * "Internal server error" message. */ + details = `Received RST_STREAM with code ${stream.rstCode} (Internal server error)`; + } else { + /* The "Received RST_STREAM with code ..." error is preserved + * here for continuity with errors reported online, but the + * error message at the end will probably be more relevant in + * most cases. */ + details = `Received RST_STREAM with code ${stream.rstCode} triggered by internal client error: ${this.internalErrorMessage}`; + } + break; + default: + code = Status.INTERNAL; + details = `Received RST_STREAM with code ${stream.rstCode}`; + } + // This is a no-op if trailers were received at all. + // This is OK, because status codes emitted here correspond to more + // catastrophic issues that prevent us from receiving trailers in the + // first place. + this.endCall({ code, details, metadata: new Metadata() }); + }); }); - stream.on('error', (err: Error) => { + stream.on('error', (err: NodeError) => { /* We need an error handler here to stop "Uncaught Error" exceptions * from bubbling up. However, errors here should all correspond to * "close" events, where we will handle the error more granularly */ + /* Specifically looking for stream errors that were *not* constructed + * from a RST_STREAM response here: + * https://github.com/nodejs/node/blob/8b8620d580314050175983402dfddf2674e8e22a/lib/internal/http2/core.js#L2267 + */ + if (err.code !== 'ERR_HTTP2_STREAM_ERROR') { + this.internalErrorMessage = err.message; + } }); if (!this.pendingRead) { stream.pause();