-
Notifications
You must be signed in to change notification settings - Fork 819
Implement SerializationContext buffer writer #629
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.
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
Implement SerializationContext buffer writer #629
Conversation
293b9b5 to
80b18d9
Compare
|
Before: After: |
|
Wow, it looks like you implemented option 3; gotta say: I'm impressed. Way to over-deliver! |
JunTaoLuo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments
src/Grpc.AspNetCore.Server/Internal/HttpContextSerializationContext.cs
Outdated
Show resolved
Hide resolved
jtattermusch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks goods but had a few questions.
| } | ||
| else | ||
| { | ||
| GrpcServerLog.SerializedMessage(_serverCallContext.Logger, _serverCallContext.MethodContext.ResponseType, _payloadLength.GetValueOrDefault()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think upon completion we should check that we've actually written the right amount of bytes to match the announced message size, otherwise we will get a protocol error at the peer (and that's something that's potentially hard to debug?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose not to because this code will only be overridden by frameworks. They should test they are doing the right thing themselves using unit/functional tests.
Checking every serialization feels wasteful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to commit this as it is. If you think this is important, or there are bug reports around this issue after release, then it can be added later by wrapping the buffer writer returned.
src/Grpc.AspNetCore.Server/Internal/HttpContextSerializationContext.cs
Outdated
Show resolved
Hide resolved
…ArrayBufferWriter<byte>
…; also allow buffer-writers to be reused (but lazily created)
4ee266b to
a592761
Compare
a592761 to
ecc8f79
Compare
|
This seems to have broken compatibility with grpc-java. I think it is because the gzip stream isn't "finish"ed. We can discuss on grpc/grpc#20884. |
Fixes #609
Fixes #608
@mgravell