protobuf-net / protobuf-net.Grpc

GRPC bindings for protobuf-net and grpc-dotnet
Other
867 stars 114 forks source link

Arg_NotSupportedException when deserializing ImmutableArray with Assembly Trimming #190

Open JakenVeina opened 3 years ago

JakenVeina commented 3 years ago

This may or may not be a bug in the protobuf-net library, but I figured this was the best place to start, Happy to escalate elsewhere, if needed.

I'm developing an application that consists of an ASP.NET Core server and a Blazor WebAssembly client, which communicate using protobuf-net.Grpc. After deploying the first live production beta, I found that a variety of gRPC endpoints were throwing exceptions in the client, but only in the live production version.

crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100]
      Unhandled exception rendering component: Status(StatusCode="Internal", Detail="Error starting gRPC call. NotSupportedException: Arg_NotSupportedException", DebugException="System.NotSupportedException: Arg_NotSupportedException
         at System.Collections.Immutable.ImmutableArray`1[[BlazorTrimmingIssue.Shared.ScalarResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].System.Collections.Generic.ICollection<T>.Add(ScalarResponse )
         at ProtoBuf.Serializers.CompiledSerializer.ProtoBuf.Serializers.IProtoSerializer.Read(Object value, ProtoReader source)
         at ProtoBuf.Meta.RuntimeTypeModel.Deserialize(Int32 key, Object value, ProtoReader source)
         at ProtoBuf.Meta.TypeModel.DeserializeCore(ProtoReader reader, Type type, Object value, Boolean noAutoCreate)
         at ProtoBuf.Meta.TypeModel.Deserialize(Stream source, Object value, Type type, SerializationContext context)
         at ProtoBuf.Meta.TypeModel.ProtoBuf.IProtoInput<System.ArraySegment<System.Byte>>.Deserialize[VectorResponse](ArraySegment`1 source, VectorResponse value, Object userState)
         at ProtoBuf.Grpc.Configuration.ProtoBufMarshallerFactory.ContextualDeserialize[VectorResponse](DeserializationContext context)
         at Grpc.Net.Client.StreamExtensions.<ReadMessageAsync>d__4`1[[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext()
         at Grpc.Net.Client.Internal.GrpcCall`2.<RunCall>d__70[[ProtoBuf.Grpc.Internal.Empty, protobuf-net.Grpc, Version=1.0.0.0, Culture=neutral, PublicKeyToken=257b51d87d2e4d67],[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext()")
Grpc.Core.RpcException: Status(StatusCode="Internal", Detail="Error starting gRPC call. NotSupportedException: Arg_NotSupportedException", DebugException="System.NotSupportedException: Arg_NotSupportedException
   at System.Collections.Immutable.ImmutableArray`1[[BlazorTrimmingIssue.Shared.ScalarResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].System.Collections.Generic.ICollection<T>.Add(ScalarResponse )
   at ProtoBuf.Serializers.CompiledSerializer.ProtoBuf.Serializers.IProtoSerializer.Read(Object value, ProtoReader source)
   at ProtoBuf.Meta.RuntimeTypeModel.Deserialize(Int32 key, Object value, ProtoReader source)
   at ProtoBuf.Meta.TypeModel.DeserializeCore(ProtoReader reader, Type type, Object value, Boolean noAutoCreate)
   at ProtoBuf.Meta.TypeModel.Deserialize(Stream source, Object value, Type type, SerializationContext context)
   at ProtoBuf.Meta.TypeModel.ProtoBuf.IProtoInput<System.ArraySegment<System.Byte>>.Deserialize[VectorResponse](ArraySegment`1 source, VectorResponse value, Object userState)
   at ProtoBuf.Grpc.Configuration.ProtoBufMarshallerFactory.ContextualDeserialize[VectorResponse](DeserializationContext context)
   at Grpc.Net.Client.StreamExtensions.<ReadMessageAsync>d__4`1[[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext()
   at Grpc.Net.Client.Internal.GrpcCall`2.<RunCall>d__70[[ProtoBuf.Grpc.Internal.Empty, protobuf-net.Grpc, Version=1.0.0.0, Culture=neutral, PublicKeyToken=257b51d87d2e4d67],[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext()")
   at ProtoBuf.Grpc.Internal.Reshape.<UnaryTaskAsyncImpl>d__12`2[[ProtoBuf.Grpc.Internal.Empty, protobuf-net.Grpc, Version=1.0.0.0, Culture=neutral, PublicKeyToken=257b51d87d2e4d67],[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext()
   at BlazorTrimmingIssue.Client.ApplicationRootModel.<TestVectorResponseAsync>b__25_0()
   at BlazorTrimmingIssue.Client.ApplicationRootModel.DoTestAsync(Func`1 testAsync)
   at Microsoft.AspNetCore.Components.ComponentBase.CallStateHasChangedOnAsyncCompletion(Task task)
   at Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle)

I was able to narrow down the issue to use of ImmutableArray<T> in the model layer, AND the fact that the web client was being published with Assembly trimming enabled (which is enabled by default for Blazor apps). Removing either of these elements from the scenario eliminates the problem.

I've assembled a minimal codebase that reproduces the issue, here.

I've also managed to find a workaround for the issue, by upgrading the project to net6.0, where it's possible to disable trimming on a per-assembly basis, and disabling trimming for System.Collections.Immutable. I've checked this in as a separate branch to the codebase.

Between the exception stack trace, and the fact that disabling trimming for System.Collections.Immutable fixes the issue, it seems like something is getting trimmed off of the ImmutableArray<T> type, which causes the protobuf library to try and deserialize it as an ICollection<T>, which, by design, throws a NotSupportedException.

mgravell commented 3 years ago

This is very interesting. More of a protobuf-net concern than protobuf-net.Grpc, but that's not hugely important. I don't know what it has done here; the type is clearly of the concrete type,and we have a handler specifically for that concrete type, so I don't know how it has got to the ICollection-T Add method (which: won't work).

Unfortunately I'm a bit scuppered today as my house electrics have failed - waiting on electrician. I'm also not sure where I'd even start unpicking this, but: :shrug:

On Tue, 20 Jul 2021, 06:50 Jake Meiergerd, @.***> wrote:

This may or may not be a bug in the protobuf-net library, but I figured this was the best place to start, Happy to escalate elsewhere, if needed.

I'm developing an application that consists of an ASP.NET Core server and a Blazor WebAssembly client, which communicate using protobuf-net.Grpc. After deploying the first live production beta, I found that a variety of gRPC endpoints were throwing exceptions in the client, but only in the live production version.

crit: Microsoft.AspNetCore.Components.WebAssembly.Rendering.WebAssemblyRenderer[100] Unhandled exception rendering component: Status(StatusCode="Internal", Detail="Error starting gRPC call. NotSupportedException: Arg_NotSupportedException", DebugException="System.NotSupportedException: Arg_NotSupportedException at System.Collections.Immutable.ImmutableArray1[[BlazorTrimmingIssue.Shared.ScalarResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].System.Collections.Generic.ICollection<T>.Add(ScalarResponse ) at ProtoBuf.Serializers.CompiledSerializer.ProtoBuf.Serializers.IProtoSerializer.Read(Object value, ProtoReader source) at ProtoBuf.Meta.RuntimeTypeModel.Deserialize(Int32 key, Object value, ProtoReader source) at ProtoBuf.Meta.TypeModel.DeserializeCore(ProtoReader reader, Type type, Object value, Boolean noAutoCreate) at ProtoBuf.Meta.TypeModel.Deserialize(Stream source, Object value, Type type, SerializationContext context) at ProtoBuf.Meta.TypeModel.ProtoBuf.IProtoInput<System.ArraySegment<System.Byte>>.Deserialize[VectorResponse](ArraySegment1 source, VectorResponse value, Object userState) at ProtoBuf.Grpc.Configuration.ProtoBufMarshallerFactory.ContextualDeserialize[VectorResponse](DeserializationContext context) at Grpc.Net.Client.StreamExtensions.d41[[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext() at Grpc.Net.Client.Internal.GrpcCall2.d70[[ProtoBuf.Grpc.Internal.Empty, protobuf-net.Grpc, Version=1.0.0.0, Culture=neutral, PublicKeyToken=257b51d87d2e4d67],[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext()") Grpc.Core.RpcException: Status(StatusCode="Internal", Detail="Error starting gRPC call. NotSupportedException: Arg_NotSupportedException", DebugException="System.NotSupportedException: Arg_NotSupportedException at System.Collections.Immutable.ImmutableArray1[[BlazorTrimmingIssue.Shared.ScalarResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].System.Collections.Generic.ICollection<T>.Add(ScalarResponse ) at ProtoBuf.Serializers.CompiledSerializer.ProtoBuf.Serializers.IProtoSerializer.Read(Object value, ProtoReader source) at ProtoBuf.Meta.RuntimeTypeModel.Deserialize(Int32 key, Object value, ProtoReader source) at ProtoBuf.Meta.TypeModel.DeserializeCore(ProtoReader reader, Type type, Object value, Boolean noAutoCreate) at ProtoBuf.Meta.TypeModel.Deserialize(Stream source, Object value, Type type, SerializationContext context) at ProtoBuf.Meta.TypeModel.ProtoBuf.IProtoInput<System.ArraySegment<System.Byte>>.Deserialize[VectorResponse](ArraySegment1 source, VectorResponse value, Object userState) at ProtoBuf.Grpc.Configuration.ProtoBufMarshallerFactory.ContextualDeserialize[VectorResponse](DeserializationContext context) at Grpc.Net.Client.StreamExtensions.d41[[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext() at Grpc.Net.Client.Internal.GrpcCall2.d70[[ProtoBuf.Grpc.Internal.Empty, protobuf-net.Grpc, Version=1.0.0.0, Culture=neutral, PublicKeyToken=257b51d87d2e4d67],[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext()") at ProtoBuf.Grpc.Internal.Reshape.d12`2[[ProtoBuf.Grpc.Internal.Empty, protobuf-net.Grpc, Version=1.0.0.0, Culture=neutral, PublicKeyToken=257b51d87d2e4d67],[BlazorTrimmingIssue.Shared.VectorResponse, BlazorTrimmingIssue.Shared, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]].MoveNext() at BlazorTrimmingIssue.Client.ApplicationRootModel.b25_0() at BlazorTrimmingIssue.Client.ApplicationRootModel.DoTestAsync(Func`1 testAsync) at Microsoft.AspNetCore.Components.ComponentBase.CallStateHasChangedOnAsyncCompletion(Task task) at Microsoft.AspNetCore.Components.RenderTree.Renderer.GetErrorHandledTask(Task taskToHandle)

I was able to narrow down the issue to use of ImmutableArray in the model layer, AND the fact that the web client was being published with Assembly trimming https://docs.microsoft.com/en-us/dotnet/core/deploying/trim-self-contained enabled (which is enabled by default for Blazor apps). Removing either of these elements from the scenario eliminates the problem.

I've assembled a minimal codebase that reproduces the issue, here https://github.com/JakenVeina/BlazorTrimmingIssue.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/protobuf-net/protobuf-net.Grpc/issues/190, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMBR5P5TWDRI5XR5UK3TYUFA3ANCNFSM5AVCFY4A .

mgravell commented 3 years ago

Just dragging this fix in from your branch for visibility reasons:

  <Target Name="ConfigureTrimming"
          BeforeTargets="PrepareForILLink">
    <ItemGroup>
      <ManagedAssemblyToLink Condition="'%(Filename)' == 'System.Collections.Immutable'">
        <IsTrimmable>false</IsTrimmable>
      </ManagedAssemblyToLink>
    </ItemGroup>
  </Target>

I wonder if this is simply the correct "fix", and we just need to communicate this?

chucker commented 3 years ago

Could it be that this isn't trimming, but rather protobuf is trying to mutate the array? I see NotSupportedExceptions here: https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs

mgravell commented 3 years ago

Yeah, but it picks the correct implementation (we have specific code for immutable-array etc) when trimming isn't active

MichalStrehovsky commented 3 years ago

I would check what methods are left on the ImmutableArray and the related builder after trimming (look at the DLL in ILSpy). It's possible a necessary method was stripped.

I can see that some of the places in protobuf-net have dataflow annotations (DynamicallyAccessedMembersAttribute), but it seems like the dynamically-accessed-annotated-methods then get invoked through reflection. That's a no-no because illinker doesn't see the invocation and can't make sure the invariants hold (normally illink would ensure that whatever arguments you use to invoke the annotated method have a proper annotation or get their expected members kept, but illink can't see through MethodInfo.Invoke). There's an open pull request in illinker to warn whenever an annotated method becomes a visible target of reflection.

JakenVeina commented 3 years ago

Could it be that this isn't trimming, but rather protobuf is trying to mutate the array? I see NotSupportedExceptions here: https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/ImmutableArray_1.cs

I mean, that is definitely what's happening, the question is why, and why is it only when the System.Collections.Immutable assembly is trimmed?

I would check what methods are left on the ImmutableArray and the related builder after trimming (look at the DLL in ILSpy). It's possible a necessary method was stripped.

I was wondering that myself, so I had a look last night. I did notice that the .Add() method is trimmed, while the ICollection<T>.Add() method remains. It certainly could be that the wrong method is being called, because the correct one has been removed, but it's tough for me to say, since the exception seems to be coming out of dynamically-emitted IL.

eerhardt commented 3 years ago

I added the following to the BlazorTrimmingIssue.Client.csproj and the issue stopped reproducing for me:

<PackageReference Include="protobuf-net" Version="3.0.101" />

I noticed that the protobuf-net version used in the app was quite old - 2.4.6. I think that is because the latest https://www.nuget.org/packages/protobuf-net.Grpc/ depends on 2.4.6 of protobuf-net:

image

eerhardt commented 3 years ago

I would check what methods are left on the ImmutableArray and the related builder after trimming

Here is the diff I see between trimmed and untrimmed:

image

mgravell commented 3 years ago

@eerhardt that's a good spot; and indeed, the immutable handling between V2 and V3 is radically different, with V3 being far more "obvious" in terms of what APIs it uses, so: maybe that's the real fix here: use V3 when using Blazor.

eerhardt commented 3 years ago

@mgravell - any reason the latest version of protobuf-net.Grpc doesn't reference the latest protobuf-net?

mgravell commented 3 years ago

Because there are a couple of deprecated features - pretty niche, but it could be a pain point for anyone still using them.

MichalStrehovsky commented 3 years ago

I would guess it gets lost somewhere around here:

https://github.com/protobuf-net/protobuf-net/blob/2a6c8a4bdde0bcf37361aa4fc4424cc43293bac4/src/protobuf-net/Serializers/ImmutableCollectionDecorator.cs#L95-L114

Illink has little chance to figure this string concatenation pattern out and it looks like it will just move on quietly if it didn't find the thing it's looking for.

akoeplinger commented 3 years ago

This shouldn't be Blazor specific btw., any net6.0 app that enables trimming should hit it.

JakenVeina commented 3 years ago

Here is the diff I see between trimmed and untrimmed: @eerhardt Here is the diff I see between trimmed and untrimmed:

Is there a tool you used to generate that, or did you just extract text from the two assemblies, and do a text diff?

@eerhardt that's a good spot; and indeed, the immutable handling between V2 and V3 is radically different, with V3 being far more "obvious" in terms of what APIs it uses, so: maybe that's the real fix here: use V3 when using Blazor.

Fine by me, so long as it's not gonna cause breaking changes in protobuf-net.Grpc.

@mgravell Because there are a couple of deprecated features - pretty niche, but it could be a pain point for anyone still using them.

For the sake of due diligence, what features would I, or anyone else, want to watch out for?

Also, any thoughts on when a new version of protobuf-net.Grpc might publish?

Thanks a lot, everyone, for your time and effort!

eerhardt commented 3 years ago

I used an internal tool we have named ApiReviewer. It has the ability to diff APIs (even internal and private) between two versions of assemblies.

mgravell commented 3 years ago

For the sake of due diligence, what features would I, or anyone else, want to watch out for?

https://github.com/protobuf-net/protobuf-net/blob/main/docs/3_0.md#changes-to-existing-features