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

Annotate SignalR client for native AOT #56079

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Jun 4, 2024

Addresses the following problems:

  1. Fix up some infrastructure around TrimmingAttributes to be cleaner.
    • Don't need to condition when to include them. Just always include them if you need it, and the TFM checks will be done for you.
    • Remove duplicate, standalone attribute files
  2. HubConnection and ReflectionHelper's usage of finding IAsyncEnumerable interface
  3. JsonHubProtocol using JsonSerializer following the same approach as we do in Minimal APIs
    • The only way to make this reliable is to use the System.Text.Json source generator. And the only way to do that is to configure the TypeInfoResolver on the JsonHubProtocolOptions.PayloadSerializerOptions. We have to suppress the warnings here and let the runtime exception catch cases where the source generator isn't being used.
  4. HubConnection's usage of MakeGenericMethod when using a streaming reader (IAsyncEnumerable or ChannelReader). - The only idea I have here is to follow the same approach as we do in DependencyInjection and elsewhere, which is to check for IsDynamicCodeSupported == false && IsValueType and throw an exception. This enables people to get exceptions during F5, and not only after publishing.

Alternative approach

One idea I had to support streaming ValueTypes is to fall back to pure reflection over the IAsyncEnumerable and ChannelReader objects when we are in native AOT and the item is a ValueType. I didn't feel this was worth it, since the odds someone is using ValueTypes here is low. We can always implement that later, if we see a need.

Addresses the following problems:

0. Fix up some infrastructure around TrimmingAttributes to be cleaner.
    - Don't need to condition when to include them. Just always include them if you need it, and the TFM checks will be done for you.
    - Remove duplicate, standalone attribute files
1. HubConnection and ReflectionHelper's usage of finding IAsyncEnumerable interface
2. HubConnection's usage of MakeGenericMethod when using a streaming reader (IAsyncEnumerable or ChannelReader).
    - The only idea I have here is to follow the same approach as we do in DependencyInjection and elsewhere, which is to check for `IsDynamicCodeSupported == false` && `IsValueType` and throw an exception. This enables people to get exceptions during F5, and not only after publishing.
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Jun 4, 2024
@gfoidl gfoidl added area-signalr Includes: SignalR clients and servers NativeAOT and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Jun 5, 2024
@BrennanConroy
Copy link
Member

the odds someone is using ValueTypes here is low

Odd statement to make, do you have data here that points to this? I'd be surprised if value types were uncommon.

public static bool IsIAsyncEnumerable(Type type) => GetIAsyncEnumerableInterface(type) is not null;

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
Justification = "The 'IAsyncEnumerable<>' Type must exist and so trimmer kept it. In which case " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'IAsyncEnumerable<>' Type must exist

Is this because we explicitly reference it via typeof(IAsyncEnumerable<>) below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. This method references typeof(IAsyncEnumerable<>). If this method is kept, then IAsyncEnumerable<> will be kept as well.

@@ -47,26 +47,28 @@ public static bool TryGetStreamType(Type streamType, [NotNullWhen(true)] out Typ
return false;
}

public static bool IsIAsyncEnumerable([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)] Type type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was wrong here? Was this path not being hit by a trimmed library so the annotation was just wrong?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The annotation wasn't "wrong" necessarily. The problem was that callers of this API can't guarantee statically which Types are being passed in - they use someObject.GetType() to obtain the type. So they were getting warnings they can't address.

This method doesn't need all interfaces on the Type. It just needs to check for 1 interface - IAsyncEnumerable. And so referencing that interface directly solves the trimming concern. The trimmer can't remove this interface, because it is being referenced directly.

@@ -914,6 +911,15 @@ private static HubMessage ApplyHeaders(HubMessage message, Dictionary<string, st
return message;
}

[UnconditionalSuppressMessage("Trimming", "IL2026:RequiresUnreferencedCode",
Justification = "The 'JsonSerializer.IsReflectionEnabledByDefault' feature switch, which is set to false by default for trimmed .NET apps, ensures the JsonSerializer doesn't use Reflection.")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the exception the user gets back? Is it actionable? i.e. does it mention what type caused the problem?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You get 2 different exceptions, depending on exactly what you did wrong.

  1. If you didn't configure the source generator at all. You get:
Failed to bind arguments received in invocation '(null)' of 'ReceiveMessage'.
      System.IO.InvalidDataException: Error binding arguments. Make sure that the types of the provided values match the types of the hub method being invoked.
       ---> System.InvalidOperationException: Reflection-based serialization has been disabled for this application. Either use the source generator APIs or explicitly configure the 'JsonSerializerOptions.TypeInfoResolver' property.
         at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_JsonSerializerIsReflectionDisabled()
         at System.Text.Json.JsonSerializerOptions.ConfigureForJsonSerializer()
         at System.Text.Json.JsonSerializer.GetTypeInfo(JsonSerializerOptions options, Type inputType)
         at System.Text.Json.JsonSerializer.Deserialize(Utf8JsonReader& reader, Type returnType, JsonSerializerOptions options)
         at Microsoft.AspNetCore.SignalR.Protocol.JsonHubProtocol.BindTypes(Utf8JsonReader& reader, IReadOnlyList`1 paramTypes)
         --- End of inner exception stack trace ---
         at Microsoft.AspNetCore.SignalR.Protocol.JsonHubProtocol.BindTypes(Utf8JsonReader& reader, IReadOnlyList`1 paramTypes)
         at Microsoft.AspNetCore.SignalR.Protocol.JsonHubProtocol.ParseMessage(ReadOnlySequence`1 input, IInvocationBinder binder)

Which tells you that you need to enable the source generator.

  1. Then once you have the source generator enabled, but didn't add the right Type to the generator, you get:
      Failed to bind arguments received in invocation '(null)' of 'ReceiveMessage'.
      System.IO.InvalidDataException: Error binding arguments. Make sure that the types of the provided values match the types of the hub method being invoked.
       ---> System.NotSupportedException: JsonTypeInfo metadata for type 'System.String' was not provided by TypeInfoResolver of type '[SignalRWorker.AppJsonSerializerContext]'. If using source generation, ensure that all root types passed to the serializer have been annotated with 'JsonSerializableAttribute', along with any types that might be serialized polymorphically.
         at System.Text.Json.ThrowHelper.ThrowNotSupportedException_NoMetadataForType(Type type, IJsonTypeInfoResolver resolver)
         at System.Text.Json.JsonSerializerOptions.GetTypeInfoInternal(Type type, Boolean ensureConfigured, Nullable`1 ensureNotNull, Boolean resolveIfMutable, Boolean fallBackToNearestAncestorType)
         at System.Text.Json.JsonSerializerOptions.GetTypeInfoForRootType(Type type, Boolean fallBackToNearestAncestorType)
         at System.Text.Json.JsonSerializer.Deserialize(Utf8JsonReader& reader, Type returnType, JsonSerializerOptions options)
         at Microsoft.AspNetCore.SignalR.Protocol.JsonHubProtocol.BindTypes(Utf8JsonReader& reader, IReadOnlyList`1 paramTypes)
         --- End of inner exception stack trace ---
         at Microsoft.AspNetCore.SignalR.Protocol.JsonHubProtocol.BindTypes(Utf8JsonReader& reader, IReadOnlyList`1 paramTypes)
         at Microsoft.AspNetCore.SignalR.Protocol.JsonHubProtocol.ParseMessage(ReadOnlySequence`1 input, IInvocationBinder binder)

@eerhardt
Copy link
Member Author

eerhardt commented Jun 6, 2024

the odds someone is using ValueTypes here is low
Odd statement to make, do you have data here that points to this? I'd be surprised if value types were uncommon.

Unfortunately I don't have any data here. Do you know how to get some?

My opinion is that we don't try to solve this case preemptively. If/when we get feedback that it's important to support IAsyncEnumerable<> and ChannelReader<> with value types, then I think we can do the work.

Thoughts?

@BrennanConroy
Copy link
Member

Can we make streams work by using object when it's a value type? They're going to get boxed anyways when we set them in StreamItem. And if we do that, we might as well remove MakeGenericMethod entirely? I think we talked about this a few months ago as a possible approach.

@eerhardt
Copy link
Member Author

eerhardt commented Jun 7, 2024

Can we make streams work by using object when it's a value type?

From my understanding, not without a bunch of reflection code to call methods on IAsyncEnumerable<T> and ChannelReader<T>. Since those types don't implement non-generic base classes or interfaces, there is no way to invoke methods on them without "jumping" to a <T> generic method or by using reflection to invoke the MoveNextAsync() and WaitToReadAsync() methods.

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider preemptively filing an issue for value type streaming that we'll backlog waiting for feedback?

@eerhardt
Copy link
Member Author

Consider preemptively filing an issue for value type streaming that we'll backlog waiting for feedback?

Done. #56179

@eerhardt eerhardt merged commit f7d5104 into dotnet:main Jun 10, 2024
26 checks passed
@eerhardt eerhardt deleted the SignalRNativeAOT branch June 10, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers NativeAOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants