Skip to content
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

C# - fix #19470 - emulate legacy serializer/deserializer when contextual API is used #19471

Closed

Conversation

mgravell
Copy link
Contributor

ref #19470

  1. ensure that .Serializer/.Deserializer do something useful when the Marshaller<T> is created with contextual implementations
  2. also use sane default implementation of PayloadAsReadOnlySequence, as not all contexts might implement correctly

This is because grpc-dotnet uses .Serializer/.Deserializer directly, see grpc/grpc-dotnet#357

@mgravell mgravell changed the title fix #19470 - emulate legacy serializer/deserializer when contextual API is used C# - fix #19470 - emulate legacy serializer/deserializer when contextual API is used Jun 26, 2019
@jtattermusch
Copy link
Contributor

jtattermusch commented Jul 3, 2019

The thing is that we used to have Marshallers emulate legacy serializer/deserializer when it was constructed using the contextual API, but we removed that emulation as no-longer-needed
when Grpc.Core switched from using the ContextualSerializer and ContextualDeserializer internally.

Because the contextual API is more powerful and flexible, I think it make more sense to require all gRPC implementations to use marshaller.ContextualSerializer and marshaller.ContextualDeserializer internally. Legacy code that still uses the simple serializers/deserializers (e.g. the gRPC generated code at this point) will continue to work because of the one-way emulation that marshallers provide, but I don't see a reason to continue emulating the other way.

See this PR for context: https://github.com/grpc/grpc/pull/17167/files#diff-2058a085f39531e466d41c163feccbe5

@jtattermusch jtattermusch self-assigned this Jul 3, 2019
@jtattermusch jtattermusch self-requested a review July 3, 2019 18:49
@mgravell
Copy link
Contributor Author

mgravell commented Jul 3, 2019

In that case, that's a topic for @JamesNK, 'cos right now: that isn't the case for grpc-dotnet

@jtattermusch
Copy link
Contributor

I think the right solution is to switch grpc-dotnet to use Contextual API internally (as captured in grpc/grpc-dotnet#357).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants