Skip to content

Make it possible to resolve the method info from the specific delegate type when using AddRouteHandler #53171

@davidfowl

Description

@davidfowl
Member

For more NativeAOT size savings, we can avoid preserving all type information for methods pointed to by delegates if we enable this. See dotnet/runtime#96166 (comment) for more details.

We essentially need to add another thunk here to allow resolution of MethodInfo from the delegate when using RDG (the request delegate generator).

cc @captainsafia

Activity

ghost added
area-minimalIncludes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
on Jan 5, 2024
captainsafia

captainsafia commented on Jan 5, 2024

@captainsafia
Member

We essentially need to add another thunk here to allow resolution of MethodInfo from the delegate when using RDG (the request delegate generator).

Don't you mean RDF here (compile-time generation)?

I think the proposed API would look like this:

    public RouteHandlerBuilder AddRouteHandler(
        RoutePattern pattern,
        Delegate routeHandler,
        IEnumerable<string>? httpMethods,
        bool isFallback,
        Func<MethodInfo, RequestDelegateFactoryOptions?, RequestDelegateMetadataResult>? inferMetadataFunc,
-        Func<Delegate, RequestDelegateFactoryOptions, RequestDelegateMetadataResult?, RequestDelegateResult> createHandlerRequestDelegateFunc,
+        Func<RequestDelegateFactoryOptions, RequestDelegateMetadataResult?, RequestDelegateResult> createHandlerRequestDelegateFunc,
+        Func<Delegate, MethodInfo> resolveMethodInfo)
jkotas

jkotas commented on Jan 5, 2024

@jkotas
Member

Is the whole purpose of the extra argument to provide routeHandler.Method? Can the extra argument be MethodInfo instead?

davidfowl

davidfowl commented on Jan 5, 2024

@davidfowl
MemberAuthor

Actually, you're right, we might just be able to pass it in directly 😄

captainsafia

captainsafia commented on Jan 5, 2024

@captainsafia
Member

Correct, indeed. Here's the full API proposal.

Proposed API

namespace Microsoft.AspNetCore.Routing;

public static class RouteHandlerServices
{
    public static RouteHandlerBuilder Map(
            IEndpointRouteBuilder endpoints,
            string pattern,
            Delegate handler,
            IEnumerable<string>? httpMethods,
            Func<MethodInfo, RequestDelegateFactoryOptions?, RequestDelegateMetadataResult> populateMetadata,
            Func<Delegate, RequestDelegateFactoryOptions, RequestDelegateMetadataResult?, RequestDelegateResult> createRequestDelegate,
+          MethodInfo? methodInfo)
}

Usage Examples

Example taken from code generated by RDG.

internal static RouteHandlerBuilder MapCore(
            this IEndpointRouteBuilder routes,
            string pattern,
            Delegate handler,
            IEnumerable<string>? httpMethods,
            MetadataPopulator populateMetadata,
            RequestDelegateFactoryFunc createRequestDelegate,
            MethodInfo methodInfo)
        {
            return RouteHandlerServices.Map(routes, pattern, handler, httpMethods, populateMetadata, createRequestDelegate, methodInfo);
        }
added
api-ready-for-reviewAPI is ready for formal API review - https://github.com/dotnet/apireviews
on Jan 5, 2024
ghost

ghost commented on Jan 5, 2024

@ghost

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.
added this to the 9.0-preview1 milestone on Jan 5, 2024
halter73

halter73 commented on Jan 8, 2024

@halter73
Member

API Review Notes:

  • Even though it's pretty much internal, since it's technically public, let's keep the old overload.
  • MethodInfo should not be nullable.

API Approved. We have great hopes for how much more we can trim!

namespace Microsoft.AspNetCore.Routing;

public static class RouteHandlerServices
{
    public static RouteHandlerBuilder Map(
            IEndpointRouteBuilder endpoints,
            string pattern,
            Delegate handler,
            IEnumerable<string>? httpMethods,
            Func<MethodInfo, RequestDelegateFactoryOptions?, RequestDelegateMetadataResult> populateMetadata,
            Func<Delegate, RequestDelegateFactoryOptions, RequestDelegateMetadataResult?, RequestDelegateResult> createRequestDelegate);

+    public static RouteHandlerBuilder Map(
+            IEndpointRouteBuilder endpoints,
+            string pattern,
+            Delegate handler,
+            IEnumerable<string>? httpMethods,
+            Func<MethodInfo, RequestDelegateFactoryOptions?, RequestDelegateMetadataResult> populateMetadata,
+            Func<Delegate, RequestDelegateFactoryOptions, RequestDelegateMetadataResult?, RequestDelegateResult> createRequestDelegate,
+            MethodInfo methodInfo);
}
added
api-approvedAPI was approved in API review, it can be implemented
and removed
api-ready-for-reviewAPI is ready for formal API review - https://github.com/dotnet/apireviews
on Jan 8, 2024
davidfowl

davidfowl commented on Jan 8, 2024

@davidfowl
MemberAuthor

Any reason this isn't an optional parameter?

halter73

halter73 commented on Jan 9, 2024

@halter73
Member

Any reason this isn't an optional parameter?

We were trying to avoid any binary breaks on principle, but I think it could actually matter if you had RDG endpoints compiled against .NET 8 running on .NET 9.

davidfowl

davidfowl commented on Jan 9, 2024

@davidfowl
MemberAuthor

I thought there was a way to do both, but it's escaping me right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    NativeAOTapi-approvedAPI was approved in API review, it can be implementedarea-minimalIncludes minimal APIs, endpoint filters, parameter binding, request delegate generator etc

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Participants

      @halter73@davidfowl@captainsafia@jkotas

      Issue actions

        Make it possible to resolve the method info from the specific delegate type when using AddRouteHandler · Issue #53171 · dotnet/aspnetcore