-
Notifications
You must be signed in to change notification settings - Fork 164
Enable RPC marshalable objects in NerdbankMessagePackFormatter
#1260
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
Enable RPC marshalable objects in NerdbankMessagePackFormatter
#1260
Conversation
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.
Pull Request Overview
This PR enables RPC marshalable objects in the NerdbankMessagePackFormatter by implementing source-generated proxies and improving type safety for NativeAOT compatibility.
- Adds source-generated proxy support for
[RpcMarshalable]interfaces with optional interfaces - Introduces new
IClientProxyinterface for better type checking - Integrates PolyType for shape generation while maintaining runtime flexibility for method transformations
Reviewed Changes
Copilot reviewed 125 out of 125 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| test/StreamJsonRpc.Tests/Usings.cs | Adds global using for PolyType |
| test/StreamJsonRpc.Tests/ObserverMarshalingTests.cs | Adds GenerateShape attribute for method shapes |
| test/StreamJsonRpc.Tests/ObserverMarshalingNerdbankMessagePackTests.cs | Removes conditional compilation and adds PolyType support |
| test/StreamJsonRpc.Tests/MarshalableProxyTests.cs | Updates test interfaces with TypeShape attributes and corrects typo |
| test/StreamJsonRpc.Tests/MarshalableProxyNerdbankMessagePackTests.cs | Removes conditional compilation flags |
| test/StreamJsonRpc.Tests/JsonRpcProxyGenerationTests.cs | Adds TypeShape attributes to test interfaces |
| test/StreamJsonRpc.Tests/DisposableProxyTests.cs | Removes conditional compilation for MessagePack tests |
| test/StreamJsonRpc.Tests/AsyncEnumerableTests.cs | Adds TypeShape attributes to test interfaces |
| test/StreamJsonRpc.Tests/ArchitectureTests.cs | Updates test to allow specific generated types |
| Multiple analyzer test files | Adds TypeShape attributes and updates test expectations |
| src/StreamJsonRpc/SystemTextJsonFormatter.cs | Updates marshaling to use RpcTargetMetadata |
| src/StreamJsonRpc/RpcMarshaledContext.cs | Refactors to non-generic base class |
| src/StreamJsonRpc/Reflection/ProxyBase.cs | Major refactor adding source-generated proxy support and type checking |
| src/StreamJsonRpc/Reflection/MessageFormatterRpcMarshaledContextTracker.cs | Updates marshaling logic for new proxy system |
Comments suppressed due to low confidence (1)
test/StreamJsonRpc.Tests/JsonRpcProxyGenerationTests.cs:1
- The test method name 'Interface_HasAsyncDisposeWithoutIDisposable' is more descriptive than the previous name, correctly indicating it tests async dispose functionality.
// Copyright (c) Microsoft Corporation. All rights reserved.
src/StreamJsonRpc/Reflection/MessageFormatterRpcMarshaledContextTracker.cs
Show resolved
Hide resolved
src/StreamJsonRpc/Reflection/MessageFormatterRpcMarshaledContextTracker.cs
Show resolved
Hide resolved
120cd86 to
1362e89
Compare
This was particularly challenging because `[RpcMarshalable]` interfaces can also prescribe 'optional' interfaces which may or may not be implemented by the marshaled object, and this information must be available on the receiving end. We transmit this information (before this PR), but on the receiving end, we would previously emit a dynamic proxy that implements exactly that set of interfaces. To preserve the NativeAOT readiness of `NerdbankMessagePackFormatter` though, we need to use source-generated proxies. A marshalable interface with 8 optional additional interfaces would require 256 proxies to be source generated to have every possible combination, which is ludicrous. Instead, we emit a proxy with just the core interface and another with _all_ the optional interfaces. Receivers who want to test whether an interface is _actually_ available on the remoted object should use the new `Is` and `As<T>` methods, which the proxies can respond to based on intended implementation. This was also tricky because these optional interfaces _may_ overlap one another in method name or in base types. Since each optional interface has a number prefixed to its method names, we have to be intentional about which prefix to use for a given method that may be reachable from multiple optional interfaces. This unfortunately wasn't thoroughly thought through for dynamic proxies, so for source generated ones we harden it, at the downside of not being _entirely_ compatible with dynamic proxies. But this is a corner case which I suppose no one has hit yet anyway. I made a design choice to keep the source generated code for proxies very much ignorant as to all the policies around renaming methods. This is because it's not simple, and might evolve over time. But we can't simply evolve all source generated proxies that are compiled into other assemblies, so instead we just have them call into our library for the method transformations at runtime. That has a small runtime cost, but is probably worth retaining the ability to service any bugs we discover later. - Emit source generated proxies for `[RpcMarshalable]` interfaces _and their optional interfaces_. - Add new `IClientProxy` interface that can be used to type check proxies that may implement too many interfaces. This is deliberately separated from the pre-existing `IJsonRpcClientProxy` so that RPC isn't relevant, allowing Microsoft.ServiceHub.Framework-generated local proxies to also implement it in a similar way, such that brokered service clients need not know whether RPC is involved to do a proper type check. - Add several new diagnostic analyzers - Bump Nerdbank.MessagePack to its latest prerelease version
1362e89 to
1353ed8
Compare
This was particularly challenging because
[RpcMarshalable]interfaces can also prescribe 'optional' interfaces which may or may not be implemented by the marshaled object, and this information must be available on the receiving end. We transmit this information (before this PR), but on the receiving end, we would previously emit a dynamic proxy that implements exactly that set of interfaces. To preserve the NativeAOT readiness ofNerdbankMessagePackFormatterthough, we need to use source-generated proxies. A marshalable interface with 8 optional additional interfaces would require 256 proxies to be source generated to have every possible combination, which is ludicrous. Instead, we emit a proxy with just the core interface and another with all the optional interfaces. Receivers who want to test whether an interface is actually available on the remoted object should use the newIsandAs<T>methods, which the proxies can respond to based on intended implementation.This was also tricky because these optional interfaces may overlap one another in method name or in base types. Since each optional interface has a number prefixed to its method names, we have to be intentional about which prefix to use for a given method that may be reachable from multiple optional interfaces. This unfortunately wasn't thoroughly thought through for dynamic proxies, so for source generated ones we harden it, at the downside of not being entirely compatible with dynamic proxies. But this is a corner case which I suppose no one has hit yet anyway.
I made a design choice to keep the source generated code for proxies very much ignorant as to all the policies around renaming methods. This is because it's not simple, and might evolve over time. But we can't simply evolve all source generated proxies that are compiled into other assemblies, so instead we just have them call into our library for the method transformations at runtime. That has a small runtime cost, but is probably worth retaining the ability to service any bugs we discover later.
[RpcMarshalable]interfaces and their optional interfaces.IClientProxyinterface that can be used to type check proxies that may implement too many interfaces. This is deliberately separated from the pre-existingIJsonRpcClientProxyso that RPC isn't relevant, allowing Microsoft.ServiceHub.Framework-generated local proxies to also implement it in a similar way, such that brokered service clients need not know whether RPC is involved to do a proper type check.