Annotate SignalR server for native AOT by eerhardt · Pull Request #56460 · dotnet/aspnetcore

eerhardt · 2024-06-25T23:27:02Z

Fix SignalR Server's usage of MakeGenericMethod when using a streaming reader (IAsyncEnumerable or ChannelReader) following the same approach as the client. Add a runtime check and throw an exception when trying to stream a ValueType in native AOT.

Adjust the public annotations:

Remove RequiresUnreferencedCode from AddSignalR
Add RequiresUnreferencedCode to MessagePack and NewtonsoftJson protocols

Support trimming and AOT in DefaultHubDispatcher by adding a feature switch to turn off custom awaitable support. Update ObjectMethodExecutor to support trimming and AOT by a new method that doesn't look for custom awaitables, and uses reflection instead of Linq.Expressions. This will need a corresponding PR in https://github.com/dotnet/sdk to default the feature switch value when PublishTrimmed or PublishAot is set to true.

FYI - @agocke @MichalStrehovsky @sbomer - in case you want to take a look.

Fix SignalR Server's usage of MakeGenericMethod when using a streaming reader (IAsyncEnumerable or ChannelReader) following the same approach as the client. Add a runtime check and throw an exception when trying to stream a ValueType in native AOT. Adjust the public annotations: * Remove RequiresUnreferencedCode from AddSignalR * Add RequiresUnreferencedCode to MessagePack and NewtonsoftJson protocols Support trimming and AOT in DefaultHubDispatcher by adding a feature switch to turn off custom awaitable support. Update ObjectMethodExecutor to support trimming and AOT by a new method that doesn't look for custom awaitables, and uses reflection instead of Linq.Expressions.

eerhardt · 2024-06-25T23:58:08Z

src/SignalR/server/Core/src/Internal/TypedClientBuilder.cs

    }

-    private static IEnumerable<MethodInfo> GetAllInterfaceMethods(Type interfaceType)
+    [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2062:UnrecognizedReflectionPattern",


Note the suppressions in this file are the same as in https://github.com/dotnet/runtime/blob/3ec76324514868b0ee4cfcca43ef96a4d62d44fa/src/libraries/System.Reflection.DispatchProxy/src/System/Reflection/DispatchProxyGenerator.cs#L168-L174

* Move RequiresDynamicCode to the whole Hub<T> class, so developers always get a warning when using IHubContext<THub, T>. This helps because THub needs to be a Hub<T>, which will warn as soon as it is used. * Suppress the warning in AddSignalRCore that references HubContext<THub, T> since users will get warnings when they try using IHubContext<THub, T>.

BrennanConroy · 2024-06-26T17:27:18Z

src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs

    private readonly Func<HubLifetimeContext, Exception?, Task>? _onDisconnectedMiddleware;
    private readonly HubLifetimeManager<THub> _hubLifetimeManager;

+    [FeatureSwitchDefinition("Microsoft.AspNetCore.SignalR.Hub.CustomAwaitableSupport")]


Could probably remove Hub from the name:
Microsoft.AspNetCore.SignalR.CustomAwaitableSupport

My recommendation on feature switches is to name them in such a way in case we ever need to make them as a public API, like we did with JsonSerializer.IsReflectionEnabledByDefault. So that's the pattern I've been following lately.

See also:

Metrics Feature Switch runtime#91767 (comment)

https://github.com/dotnet/runtime/blob/d9a66070026c9133fade4e61a60d00aec170bfc4/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs#L24-L28.

FYI - I'm renaming this switch to Microsoft.AspNetCore.SignalR.Hub.IsCustomAwaitableSupported to align with the naming of other feature switches:

System.Diagnostics.Tracing.EventSource.IsSupported

System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported

System.Text.Json.JsonSerializer.IsReflectionEnabledByDefault

System.ComponentModel.TypeDescriptor.IsComObjectDescriptorSupported

~~I just don't want to reset CI right now, so will push that change after CI is done running.~~

This is now done.

BrennanConroy · 2024-06-26T22:41:54Z

src/SignalR/server/SignalR/test/NativeAotTests.cs

+        RunNativeAotTest(static async () =>
+        {
+            //System.Diagnostics.Debugger.Launch();
+            var loggerFactory = new StringLoggerFactory();


Can we remove this? If you call StartServer it already configures a logger factory that is wired up to file logging (and VS logging).

StartServer requires you to pass an ILoggerFactory in:

aspnetcore/src/Shared/SignalR/InProcessTestServer.cs

Line 57 in 0da8ea7

public static async Task<InProcessTestServer<TStartup>> StartServer(ILoggerFactory loggerFactory, Action<KestrelServerOptions> configureKestrelServerOptions = null, IDisposable disposable = null)

Also, we need to assert the logs contains the right string for the tests that return just Task/ValueTask to ensure they got called correctly.

aspnetcore/src/Shared/SignalR/FunctionalTestBase.cs

Line 40 in 250b489

public Task<InProcessTestServer<T>> StartServer<T>(Func<WriteContext, bool> expectedErrorsFilter = null, Action<KestrelServerOptions> configureKestrelServerOptions = null) where T : class

aspnetcore/src/SignalR/server/SignalR/test/HubConnectionHandlerTests.ClientResult.cs

Line 96 in 250b489

Assert.Single(TestSink.Writes.Where(write => write.EventId.Name == "ErrorDispatchingHubEvent"));

The problem with that is that this code is running in a new process (spun up by the RemoteExecutor) and so we don't have access to the instance of the Test. And things like TestSink and ITestOutputHelper aren't available in the separate process.

BrennanConroy · 2024-06-26T23:28:14Z

src/SignalR/server/Core/src/IHubContext`T.cs

 /// A context abstraction for a hub.
 /// </summary>
-public interface IHubContext<THub, T>
+public interface IHubContext<THub, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>


Did we want to put RequiresDynamicCode on this type like we did for the internal implementation?

I would, but RequiresDynamicCode can only go on methods, constructors, and classes.

aspnetcore/src/Shared/TrimmingAttributes.cs

Lines 16 to 17 in 0da8ea7

[AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Class, Inherited = false)]

internal sealed class RequiresDynamicCodeAttribute : Attribute

… other feature switch naming

BrennanConroy

Do you have a size comparison?

BrennanConroy · 2024-06-28T20:56:23Z

...t/Microsoft.AspNetCore.SignalR.TrimmingTests/Microsoft.AspNetCore.SignalR.TrimmingTests.proj

@@ -0,0 +1,9 @@
+<Project Sdk="Microsoft.NET.Sdk">


What is a .proj?

It is an MSBuild project that doesn't have a language like a .csproj or a .vbproj does. It isn't technically part of MSBuild, but just a convention for a project that executes arbitrary MSBuild targets. (in this case, it generates a new .csproj from a template, publishes it, executes the pubilshed app, and verifies the app returned exit code 100.

Another example is the helixpublish.proj that publishes tests to helix.

aspnetcore/eng/common/helixpublish.proj

Lines 15 to 19 in 8046091

<HelixWorkItem Include="WorkItem" Condition="'$(WorkItemDirectory)' != ''">

<PayloadDirectory>$(WorkItemDirectory)</PayloadDirectory>

<Command>$(WorkItemCommand)</Command>

<Timeout Condition="'$(WorkItemTimeout)' != ''">$(WorkItemTimeout)</Timeout>

</HelixWorkItem>

BrennanConroy · 2024-06-28T20:56:35Z

AspNetCore.sln

 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.SignalR.Microbenchmarks", "src\SignalR\perf\Microbenchmarks\Microsoft.AspNetCore.SignalR.Microbenchmarks.csproj", "{A6A95BEF-7E21-4D3D-921B-F77267219D27}"
 EndProject
-Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.SignalR.Tests", "src\SignalR\server\SignalR\test\Microsoft.AspNetCore.SignalR.Tests.csproj", "{4DC9C494-9867-4319-937E-5FBC0E5F5A51}"
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.SignalR.Tests", "src\SignalR\server\SignalR\test\Microsoft.AspNetCore.SignalR.Tests\Microsoft.AspNetCore.SignalR.Tests.csproj", "{4DC9C494-9867-4319-937E-5FBC0E5F5A51}"


Add src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.TrimmingTests/Microsoft.AspNetCore.SignalR.TrimmingTests.proj?

We don't put these in the .sln because they aren't "normal" projects, so you don't get intellisense editing or anything like that. Because they are self-contained executables, the UX for working on these tests is different than our other tests.

BrennanConroy · 2024-06-28T20:56:48Z

src/SignalR/SignalR.slnf

      "src\\SignalR\\server\\Core\\src\\Microsoft.AspNetCore.SignalR.Core.csproj",
      "src\\SignalR\\server\\SignalR\\src\\Microsoft.AspNetCore.SignalR.csproj",
-      "src\\SignalR\\server\\SignalR\\test\\Microsoft.AspNetCore.SignalR.Tests.csproj",
+      "src\\SignalR\\server\\SignalR\\test\\Microsoft.AspNetCore.SignalR.Tests\\Microsoft.AspNetCore.SignalR.Tests.csproj",


Add src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.TrimmingTests/Microsoft.AspNetCore.SignalR.TrimmingTests.proj?

BrennanConroy · 2024-07-01T19:19:46Z

src/SignalR/server/SignalR/test/Microsoft.AspNetCore.SignalR.TrimmingTests/TestTypedClients.cs

@@ -0,0 +1,84 @@
+// Licensed to the .NET Foundation under one or more agreements.


What mechanism is running this app?

We have test infrastructure that runs all these *.TrimmingTests.proj and *.NativeAotTests.proj projects.

They run on the build machines on the "Test" legs of CI.

You can learn more about this at https://github.com/dotnet/aspnetcore/blob/main/docs/Trimming.md#validate-trimming-behavior.

eerhardt · 2024-07-01T21:54:16Z

Do you have a size comparison?

Taking a dotnet new webapiaot project and publishing it for win-x64 on my machine, the resulting .exe is 8.73 MB (9,163,776 bytes).

Then adding the following to the app:

Program.cs

var builder = WebApplication.CreateSlimBuilder(args);
...
+builder.Services.AddSignalR();
+builder.Services.Configure<JsonHubProtocolOptions>(o =>
+{
+    o.PayloadSerializerOptions.TypeInfoResolverChain.Insert(0, AppJsonSerializerContext.Default);
+});

var app = builder.Build();
...

+app.MapHub<ChatHub>("/chathub");

app.Run();

ChatHub.cs

using Microsoft.AspNetCore.SignalR;

public class ChatHub(ILogger<ChatHub> logger) : Hub
{
    public async Task SendMessage(string user, string message)
    {
        logger.LogInformation("Received message from {user}: {message}", user, message);

        await Clients.All.SendAsync("ReceiveMessage", user, message);
    }
}

Publishing again, the resulting .exe is 10.9 MB (11,445,760 bytes).

So roughly 2.2 MB of additional size to add SignalR server. Doing a really quick sizoscope on the before and after, here are my thoughts on what could be eliminated if we wanted:

System.Text.RegularExpressions is getting pulled in, which is adding a lot of the size we cut out of the "Slim" builder. The reason it gets pulled in is because this calls AddRouting and not AddRoutingCore.

aspnetcore/src/SignalR/common/Http.Connections/src/ConnectionsDependencyInjectionExtensions.cs

Lines 21 to 23 in 0da8ea7

    
           public static IServiceCollection AddConnections(this IServiceCollection services) 
        
           { 
        
               services.AddRouting();

Super minor would be to remove the Linq usage in

aspnetcore/src/SignalR/server/Core/src/Internal/HubReflectionHelper.cs

Line 13 in 0da8ea7

public static IEnumerable<MethodInfo> GetHubMethods(Type hubType)

and

aspnetcore/src/SignalR/server/Core/src/Internal/HubMethodDescriptor.cs

Line 63 in 0da8ea7

ParameterTypes = methodExecutor.MethodParameters.Where((p, index) =>

I think we are in a good spot right now though. I opened an issue for the Regex change, since that would have the biggest benefit.

BrennanConroy · 2024-07-01T22:14:38Z

I was more asking how much this PR saves vs. not trimming SignalR.

eerhardt · 2024-07-01T22:29:56Z

I was more asking how much this PR saves vs. not trimming SignalR.

If I take the same project as above and make the following changes to the .csproj:

    <!-- <PublishAot>true</PublishAot> -->
    <PublishSingleFile>true</PublishSingleFile>

The resulting .exe is 90.1 MB.

This is the SDK portion of changes needed to make SignalR compatible with native AOT. See dotnet/aspnetcore#56460.

ghost added the area-signalr Includes: SignalR clients and servers label Jun 25, 2024

eerhardt mentioned this pull request Jun 25, 2024

[Trimming] SignalR #41704

Open

1 task

eerhardt commented Jun 25, 2024

View reviewed changes

eerhardt added 4 commits June 26, 2024 09:03

Fix LinkabilityChecker warnings

40149a1

Update warning suppressions baseline.

de32d11

Add more unit tests

aefb04d

eerhardt marked this pull request as ready for review June 26, 2024 22:18

eerhardt requested review from a team, BrennanConroy, captainsafia, halter73 and wtgodbe as code owners June 26, 2024 22:18

BrennanConroy reviewed Jun 26, 2024

View reviewed changes

build-analysis bot mentioned this pull request Jun 27, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Closed

3 tasks

eerhardt added 3 commits June 27, 2024 10:28

Move SignalR server tests down a folder to make room for new tests.

035e059

Add trimming test for using typed clients.

12c4317

Fix tests

89d363b

eerhardt mentioned this pull request Jun 27, 2024

Adding SignalR CustomAwaitable feature switch dotnet/sdk#41832

Merged

Rename the feature switch to IsCustomAwaitableSupported to align with…

95a6473

… other feature switch naming

BrennanConroy approved these changes Jul 1, 2024

View reviewed changes

eerhardt mentioned this pull request Jul 1, 2024

Consider replacing AddRouting with AddRoutingCore in AddConnections #56559

Closed

eerhardt merged commit c02ff12 into dotnet:main Jul 1, 2024

dotnet-policy-service bot added this to the 9.0-preview7 milestone Jul 1, 2024

eerhardt deleted the SignalRNativeAOT-Server branch July 1, 2024 22:04

eerhardt added a commit to dotnet/sdk that referenced this pull request Jul 9, 2024

Adding SignalR CustomAwaitable feature switch (#41832)

f7472c8

This is the SDK portion of changes needed to make SignalR compatible with native AOT. See dotnet/aspnetcore#56460.

	[AttributeUsage(AttributeTargets.Method \| AttributeTargets.Constructor \| AttributeTargets.Class, Inherited = false)]
	internal sealed class RequiresDynamicCodeAttribute : Attribute

	<HelixWorkItem Include="WorkItem" Condition="'$(WorkItemDirectory)' != ''">
	<PayloadDirectory>$(WorkItemDirectory)</PayloadDirectory>
	<Command>$(WorkItemCommand)</Command>
	<Timeout Condition="'$(WorkItemTimeout)' != ''">$(WorkItemTimeout)</Timeout>
	</HelixWorkItem>

		@@ -0,0 +1,84 @@
		// Licensed to the .NET Foundation under one or more agreements.

Conversation

eerhardt commented Jun 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrennanConroy commented Jul 1, 2024

Uh oh!

eerhardt commented Jul 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eerhardt commented Jun 25, 2024 •

edited

Loading

eerhardt Jun 27, 2024 •

edited

Loading

eerhardt Jun 27, 2024 •

edited

Loading

eerhardt commented Jul 1, 2024 •

edited

Loading