Add NerdbankMessagePackFormatter #1100
Conversation
- Upgraded several package versions in `Directory.Packages.props`, including `Microsoft.Bcl.AsyncInterfaces`, `System.Collections.Immutable`, `System.IO.Pipelines`, and `System.Text.Json`. Added `Nerdbank.MessagePack`. - Modified `nuget.config` to add a new package source for `nuget.org` and included package source mappings. - Updated `StreamJsonRpc.csproj` to target only `net8.0`, removing `net6.0`, and added a reference to `Nerdbank.MessagePack`. - Changed `Benchmarks.csproj` to target `net8.0` instead of `net6.0`. - Adjusted `StreamJsonRpc.Tests.csproj` to exclusively target `net8.0`.
Replaces the `ProgressFormatterResolver` and `AsyncEnumerableFormatterResolver` with `ProgressConverterResolver` and `AsyncEnumerableConverterResolver`.
Significant modifications to the `NerdbankMessagePackFormatter` and related classes to improve serialization context and type shape provider functionalities. - Introduced new methods in `ISerializationContextBuilder.cs` for registering various type converters, enhancing flexibility for asynchronous enumerables, duplex pipes, and streams. - Replaced `PipeFormatterResolver` with `PipeConverterResolver` in `NerdbankMessagePackFormatter` to align with new converter registration methods. - Updated `FormatterContextBuilder` to utilize new context and type shape provider structures for a more modular design.
src/StreamJsonRpc/NerdbankMessagePackFormatter.ICompositeTypeShapeProviderBuilder.cs
Outdated
Show resolved
Hide resolved
src/StreamJsonRpc/NerdbankMessagePackFormatter.ICompositeTypeShapeProviderBuilder.cs
Outdated
Show resolved
Hide resolved
src/StreamJsonRpc/NerdbankMessagePackFormatter.ISerializationContextBuilder.cs
Outdated
Show resolved
Hide resolved
…rmatterContextBuilder` class. The `NerdbankMessagePackFormatter` class has been modified to eliminate the `NBMP` namespace prefix, favoring direct usage of `MessagePack` types.
src/StreamJsonRpc/NerdbankMessagePackFormatter.FormatterContextBuilder.cs
Outdated
Show resolved
Hide resolved
|
@AArnott : Current test stats: 2157 Passed |
|
Current test results: @AArnott - Currently seeing a lot of test failures around exceptions and having difficulty tracing the issues. Many errors are caused by an NB.MP throwing b/c an exception is not serializable. While adding additional formatter configuration for the tests to pass, I'm seeing some The need to pre-register converters in the serializer without the ability to resolve and cache at runtime like legacy MessagePack-CSharp resolvers, is currently the cause of the most repeated builder calls. I suppose we could make a converter act similar to a resolver, or make some converters more open to supporting a wider set of types? Edit |
|
Benchmarks:
|
Seeing the exception type, message and stack trace usually helps me understand and suggest fixes for issues like this.
FYI I haven't looked closely at the core of your pull request yet, since I'm trying to stay 'OOF' 😉 so I can't make a firm recommendation at this point.
I recently removed the exception that is thrown when you register more converters after serialization has happened. But it still resets the converter cache so it's a runtime hit that should be avoided if possible. Is your concern mostly around generics? I'm still working on a better solution for that from NB.MP that I hope will make this simpler for you.
Historically, separating envelope from user data has been useful in protecting the integrity of the JSON-RPC protocol and our extensions, while allowing the user to have flexibility in how types are serialized. If we have just one serializer object, it seems the user will either be disallowed to customize how types are serialized that we internally must serialize, or the user's customizations could invalidate the objects we serialize for the envelope. And this set of sensitive types may change over time as we change what or how we serialize objects as part of the core protocol or the exotic types. If I'm missing the mark of what you're thinking of, please correct me.
Most of those look pretty good. The last one seems to mix in making the connection itself, which seems likely to be noisy. But I have no idea why NB.MP would make the process of connecting so much slower than the other libraries. Do you? |
Totally makes sense.
Mostly, yes. With MP-CS resolvers, the right formatter is found by working through an ordered list of resolvers for any given type. This allows for behaviors like a resolver intercepting intrinsic rpc-marhalables before they matched to a more conventional formatter. Same for things like Pipes and Streams as arguments or return types. And IAsyncEnumerable as a property on a class. With NB.MP, a type like Basically, not being able to intercept the exotic types as they are seen (for best perf) means the StreamJsonRpc consumer needs to be very explicit up-front.
Not yet :-)
Totally understandable. I should probably step away from this myself for awhile. |
|
Azure Pipelines successfully started running 1 pipeline(s). |
AArnott
force-pushed
the
dev/trippwill/nbmp-formatter
branch
from
c900c6b to
b7bc374
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
AArnott
force-pushed
the
dev/trippwill/nbmp-formatter
branch
from
b7bc374 to
d180b4c
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Also delete an unused converter
This reverts some changes and fixes a test so that it actually runs the NerdbankMessagePackFormatter through the code path that we intended to.
There was a problem hiding this comment.
I've got all tests passing now. I haven't thoroughly reviewed the root formatter file, but the rest of them look good. And the test coverage looks great. Thank you, @trippwill.
| @@ -25,7 +25,7 @@ internal static class ExceptionSerializationHelpers | |||
| private static StreamingContext Context => new StreamingContext(StreamingContextStates.Remoting); | |||
|
|
|||
| internal static T Deserialize<T>(JsonRpc jsonRpc, SerializationInfo info, TraceSource? traceSource) | |||
| where T : Exception | |||
| ////where T : Exception | |||
| { | |||
There was a problem hiding this comment.
@eiriktsarpalis I removed this because a IMessagePackConverterFactory.CreateConverter<T> method has to be able to (ultimately) call this, but the T generic parameter doesn't have this constraint.
Is there any way to resolve that so that this method (and another class) can retain this generic constraint and make it instantiable from another generic type argument that does not share that constraint, after the appropriate runtime checks, that doesn't involve reflection?
src/StreamJsonRpc/NerdbankMessagePackFormatter.RequestIdConverter.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // Look up the argument types dictionary if we saved it before. | ||
| Type paramsObjectType = paramsObject.GetType(); |
There was a problem hiding this comment.
Instead of using reflection, this method should acquire the type shape. And maybe we should just serialize it as a whole instead of breaking it up.
Add an
IJsonRpcMessageFormatterimplementation that uses Nerdbank.MessagePack. This serializer has better startup time and is AOT-safe. This alone won't make StreamJsonRpc as a whole AOT safe, but it's a step in that direction.NotImplementedException