Fix premature leaving of context due to improper Http2ServerCallStream handling #2501
Conversation
packages/grpc-js/src/server-call.ts
Outdated
| const call = this; | ||
| const body: Buffer[] = []; | ||
| const limit = this.maxReceiveMessageSize; | ||
| async function onData(chunk: Buffer) { |
There was a problem hiding this comment.
Making a function async has two main effects: it can await things and it returns a promise. This function does not await anything and it is used as an event listener so its return value is not used. So, I don't see any reason to make that change.
onEnd has the same issue.
There was a problem hiding this comment.
Sorry, this was a slight oversight. I halfway through opted to make this change as little as possible and overlooked this
packages/grpc-js/src/server-call.ts
Outdated
| resolve: ( | ||
| value: void | RequestType | PromiseLike<void | RequestType> | ||
| ) => void, | ||
| reject: (reason: any) => void |
There was a problem hiding this comment.
I don't like passing resolve and reject in here. It's not a normal async flow and it makes the code logic more complicated. It would be better for this function to return a promise itself. In fact, I think that if you make this function async and replace the entire function body with return this.deserializeMessage(buffer), any error will bubble up as a promise rejection, which can be handled by the caller. Alternatively, you could keep the try...catch, but throw the object that is currently passed to reject.
There was a problem hiding this comment.
This was in an attempt to make this method behave in the same way as before. I can see how this definitely makes it less understandable. I opted to the second option, keeping the try...catch, because I want to keep the Error mapping, and otherwise I'd have to do it twice .
Changing the implementation in this way did make the name of the method untrue, which is why I renamed it now to deserializeMessageWithInternalError. The name is a little on the longer side, but it's a private method and in my opinion definitely increases the readability.
packages/grpc-js/src/server-call.ts
Outdated
| compressedMessageEncoding | ||
| ); | ||
| if (Buffer.isBuffer(decompressedMessage)) { | ||
| call.deserializeMessageWithInternalError(decompressedMessage); |
There was a problem hiding this comment.
Now that this method returns the value, you have to do something with that value here and at the other call site. The way it currently works, I think you need another try/catch block here to pass the result into resolve or reject as appropriate. However, I think if you make deserializeMessageWithInternalError into an async function, it will automatically translate the throw there into a rejected promise. Then, since resolve here accepts a PromiseLike<RequestType | void>, you can pass the result of that call directly to resolve and it will handle both cases.
There was a problem hiding this comment.
Oh yeah you're right, I neglected the resolve there. I now turned it into an async function and specifically specified the resolve and catch at the call accordingly to make it very clear what is happening to the result. Otherwise it might become unclear to some, why deserializeMessageWithInternalError is async.
I thought if awaiting it might matter, because it would make the handling more complicated. But I don't think it makes a difference, because we're inside of another promise that only resolves after this call is finished anyway.
packages/grpc-js/src/server-call.ts
Outdated
| call | ||
| .deserializeMessageWithInternalError(decompressedMessage) | ||
| .then(resolve) | ||
| .catch(reject); |
There was a problem hiding this comment.
I'm pretty sure this can just be resolve(call.deserializeMessageWithInternalError(decompressedMessage)). The output of that function is now a promise-like object, which can be passed directly to resolve.
There was a problem hiding this comment.
I actually preferred the readability of explicitly passing resolve and reject, to actually show why we might want a Promise here, but I can also understand why you would want to keep this as simple as possible.
I'll push a change for that as well.
There was a problem hiding this comment.
From my point of view, part of the benefit of using promises is the ability to pass them around transparently in APIs that handle them. If you're concerned about communicating something to future readers, you can add a comment wherever you think it's relevant to explain it explicitly.
There was a problem hiding this comment.
I'm not really concerned, it's still very readable. I just personally prefer writing explicit code, but as I said I also understand why keeping it simplistic has its benefits, and I definitely want to adhere to the code style this repository aims to maintain.
To provide context and metadata in our internal gRPC handling framework, we utilize nodejs's asynchronous context tracking, specifically metadata namespaces similar to
cls-hooked, implemented withAsyncLocalStorage.Due to the asynchronous nature of the
Http2ServerCallStream, the synchronous handling of receiving unary messages introduced in #2249 causes unexpected behavior. Handling the stream outside of aPromisemakes node leave the context prematurely before continuing the message handling, therefore rendering access to the information stored withAsyncLocalStorageimpossible.