`RequestDelegateMetadataResult.CachedFactoryContext` is unused, resulting in unnecessary work

[andrewlock]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

I was looking through some of the code related to RequestDelegateFactory and I noticed that RequestDelegateMetadataResult.CachedFactoryContext doesn't appear to be used. I'm not sure whether this is a bug or by design, but it seems like not using it results in a lot of duplicated work building Expressions? Especially as the documentation for CachedFactoryContext says:

// This internal cached context avoids redoing unnecessary reflection in Create that was already done in InferMetadata.
// InferMetadata currently does more work than it needs to building up expression trees, but the expectation is that InferMetadata will usually be followed by Create.
internal RequestDelegateFactoryContext? CachedFactoryContext { get; set; }

Expected Behavior

I would expect the CachedFactoryContext to be set in InferMetadata, e.g something like this:

public static RequestDelegateMetadataResult InferMetadata(MethodInfo methodInfo, RequestDelegateFactoryOptions? options = null)
{
    var factoryContext = CreateFactoryContext(options);
    factoryContext.ArgumentExpressions = CreateArgumentsAndInferMetadata(methodInfo, factoryContext);

    return new RequestDelegateMetadataResult
    {
        EndpointMetadata = AsReadOnlyList(factoryContext.EndpointBuilder.Metadata),
+       CachedFactoryContext = factoryContext,
    };
}

Steps To Reproduce

I simply created a new minimal API app, debugged into RequestDelegateFactory, and placed a breakpoint in CreateArguments(). It's called both in InferMetadata() and Create(), which CachedFactoryContext is meant to avoid (if I understand correctly!)

Exceptions (if any)

No response

.NET Version

7.0.101

Anything else?

No response

[davidfowl]

@halter73 ?

[halter73]

This definitely looks like a bug. It shouldn't have a behavioral impact (given a non-null RequestDelegateMetadataResult we skip reinferring metadata with or without the cached context), but it does hurt startup time defeating the purpose of having the cached context there to begin with. I'll open a PR tomorrow with a regression test and backport it to 7. Thanks for the report @andrewlock.

[andrewlock]

Thanks @halter73 - I created a small PR to fix it here #45865

Was mostly out of interest though, so feel free to close and do it "properly" 😅