fix: properly handle http error status codes #943
Conversation
packages/grpc-js/src/call-stream.ts
Outdated
| } | ||
|
|
||
| if (flags & http2.constants.NGHTTP2_FLAG_END_STREAM) { | ||
| if (flags & http2.constants.NGHTTP2_FLAG_END_STREAM || this.mappedStatusCode !== Status.OK) { |
There was a problem hiding this comment.
The problem here is that if the END_STREAM flag is not set, then there will still be actual trailers later. I think there might be a better solution here that doesn't involve discarding those trailers.
There was a problem hiding this comment.
Yeah, I figured. I'm open to suggestions!
There was a problem hiding this comment.
The simplest solution for now would probably be to just remove that end event handler I linked to in the issue. I think that check actually doesn't exist in the other implementation.
There was a problem hiding this comment.
That doesn't work because the status code set in mappedStatusCode only gets handled in handleTrailers, which never gets called again.
There was a problem hiding this comment.
Something worth noting here is that in the case that I'm working with, the problem arrises when grpc is is being served behind nginx with grpc_pass.
In this case, nginx serves the 404 error - so I don't think the trailers ever come.
There was a problem hiding this comment.
After some further debugging it seems that the nginx proxy was incorrectly setup in my case. So, I have updated this PR to remove the changes related to the handing of the trailers - it now just fixes the http status code checking (number vs string)
mrfelton
force-pushed
the
fix/404-handling
branch
from
5ccd1d4 to
7e5dec8
Compare
packages/grpc-js/src/call-stream.ts
Outdated
| } else { | ||
| this.http2Stream = stream; | ||
| stream.on('response', (headers, flags) => { | ||
| switch (headers[HTTP2_HEADER_STATUS]) { |
There was a problem hiding this comment.
This line now has to say headers[':status'] so that TypeScript can recognize that the value will be a number.
mrfelton
force-pushed
the
fix/404-handling
branch
from
7e5dec8 to
aa54122
Compare
|
Thank you for your contribution |
|
0.5.1 is now out with this fix. |
3 participants
This is an attempt to get a fix for #941
There are 2 things here:
Correctly check http error status codes as number rather than string. This is probably correct.
Trigger
handleTrailersin the case of an http error. This is probably not the right thing to do, but I'm not very familiar with the codebase or protocol and I'm literally just hacking away trying to get the result I'm looking for. This does fix the error I'm experiencing, although probably not the right way to go about it.