grpc-js: add client streaming RPC support #859
Conversation
This commit adds client streaming RPC support.
packages/grpc-js/src/server-call.ts
Outdated
| this.cancelled = false; | ||
| this.decoder = new StreamDecoder(); | ||
|
|
||
| const http2Stream = this.call._getHttp2Stream(); |
There was a problem hiding this comment.
This really feels like an abstraction layering violation. The Http2ServerCallStream owns the http2 stream object, and all interaction with the http2 stream should go through it.
There was a problem hiding this comment.
I'm open to suggestions. I can think of a few other ways of doing this, but I'd like to avoid guessing wrong and rewriting it multiple times.
There was a problem hiding this comment.
So, I don't think that @murgatroid99 was hinting at a particular way of writing this. The gist of his remark is that if we look at the current structure of the code, you have this class you wrote a little while back named Http2ServerCallStream with a comment on it that says it's wrapping the HTTP2 request.
But now, in this new pull request, you're bypassing this wrapper, and you are accessing the ServerHttp2Stream object directly. So, from an overall design point of view, this seems inconsistent. On one hand, you're wrapping the features of ServerHttp2Stream, mutating some of this class' behavior such as the write function, and on the other hand, you're bypassing these mutations. Thinking about the future maintainer of this code, one needs precise rules and contracts about what's being wrapped and why.
If you're willing to try and bypass the wrapping you introduced yourself, would you mind sharing your thoughts on what the contracts here should be?
There was a problem hiding this comment.
I was just trying to keep the overhead low since it's an internal class. I can have the ServerHttp2Stream instance listen for 'data' and 'end' events from the http2 stream and re-emit them. Similarly, I can wrap the pause() and resume() methods. Would you prefer that, or do you have other ideas (which is what I meant by my original comment)?
There was a problem hiding this comment.
Directly listening to the 'data' event won't work well because the stream classes have special handling around that event: they will start reading as soon as you add that listener.
The strategy used on the client side is for the internal call stream class to be a duplex stream of Buffer messages, and when the surface call needs to send or receive a unary message it just writes or reads only one message. I think it would be reasonable to do that here too.
Another option I can see is to have all of the Server*Impl classes directly own and interact with the ServerHttp2Stream, with helper functions or a helper class for de-duplicating some code.
This commit adds a setupReadable() method to Http2ServerCallStream. This is used to set up the plumbing between the HTTP2 stream and the surface readable/bidi calls.
murgatroid99
left a comment
murgatroid99
left a comment
There was a problem hiding this comment.
Other than one simple change this looks good to me.
packages/grpc-js/src/server-call.ts
Outdated
| } | ||
|
|
||
| deserialize(input: Buffer): RequestType|null { | ||
| if (input === null || input === undefined) { |
There was a problem hiding this comment.
The function type declaration says this will never be null or undefined. And this is intended to only be called by our own internal code. And the code that calls it explicitly checks for null first. So this whole condition should be unnecessary.
There was a problem hiding this comment.
OK. I've removed it.
This commit removes null and undefined checks from deserialize().
4 participants
This commit adds client streaming RPC support.