ModelEndpointDataSource not idempotent

[yang-er]

Describe the bug

ModelEndpointDataSource.Endpoints.getter is not idempotent.

Since DefaultEndpointConventionBuilder.Build() doesn't use another copy / clone of the original EndpointBuilder, if the _conventions is not empty, the convention will be applied one more time. Something like endpointConventionBuilder.WithMetadata(new HttpMethodMetadata(new[] { "GET" })) will add the repeated metadata object into that EndpointBuilder, and the metadata collection of last built endpoint will grow larger.

The following scenarios will make the convention collection non-empty:

• endpoints.MapMethods/MapGet/MapPost/MapPut/MapFallback/...
• endpoints.Map(...).WithMetadata(...)

A new endpoint will be created when

• Use reflection and read IOptions<RouteOptions>.Value.EndpointDataSources.SelectMany(a => a.Endpoints)
• The change token of ActionEndpointDataSourceBase is notified (which will take the last built endpoint to Match)

Maybe we should make DefaultEndpointConventionBuilder.Build() idempotent or the result of ModelEndpointDataSource.Endpoints cached.

Further technical details

• ASP.NET Core version 3.0, 3.1, 5.0

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[mkArtakMSFT]

Thanks for contacting us.
@javiercn , @JamesNK any thoughts regarding this? Thanks!

[javiercn]

@yang-er thanks for contacting us.

Can you create a minimal repro project that illustrates the issue so that we can better understand the problem you are running into?

[yang-er]

using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.Routing.Patterns;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Primitives;
using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

namespace ReplayDemo
{
    public class Program
    {
        public static void Main(string[] args)
            => CreateHostBuilder(args).Build().Run();

        public static IHostBuilder CreateHostBuilder(string[] args) =>
            Host.CreateDefaultBuilder(args)
                .ConfigureWebHostDefaults(webBuilder =>
                    webBuilder.UseStartup<Program>());

        public void ConfigureServices(IServiceCollection services)
        {
        }

        public void Configure(IApplicationBuilder app)
        {
            app.UseDeveloperExceptionPage();
            app.UseRouting();

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapGet("/", RequestHandler);

                var my = new MyEndpointDataSource();
                endpoints.DataSources.Add(my);

                endpoints.Map("/cancel", context =>
                {
                    my.Cancel();
                    return context.Response.WriteAsync("Cleared!");
                });
            });
        }

        private static async Task RequestHandler(HttpContext context)
        {
            var endpointDataSource = context.RequestServices.GetRequiredService<EndpointDataSource>();
            if (endpointDataSource is CompositeEndpointDataSource composite)
            {
                await WriteLineAsync("Current endpoint data source is CompositeEndpointDataSource.");

                foreach (var dataSources in composite.DataSources)
                {
                    await WriteLineAsync("");
                    await WriteLineAsync(dataSources.GetType().Name);
                    foreach (var endpoint in dataSources.Endpoints)
                    {
                        await WriteLineAsync($"- {endpoint.DisplayName} (total {endpoint.Metadata.Count} metadatas)");
                    }
                }
            }

            await WriteLineAsync("");
            await WriteLineAsync("");
            var current = context.GetEndpoint();
            await WriteLineAsync($"Current endpoint: {current.DisplayName}");
            foreach (var metadata in current.Metadata)
            {
                await WriteLineAsync($"- {metadata}");
            }

            Task WriteLineAsync(string text) => context.Response.WriteAsync(text + "\n");
        }
    }

    public class MyEndpointDataSource : EndpointDataSource
    {
        private static readonly RequestDelegate DisplayCurrent = async context =>
        {
            var current = context.GetEndpoint();
            await context.Response.WriteAsync($"Current endpoint: {current.DisplayName}\n");
            foreach (var metadata in current.Metadata)
            {
                await context.Response.WriteAsync($"- {metadata}\n");
            }
        };

        private readonly object _locker = new object();
        private IReadOnlyList<Endpoint> _endpoints = null;
        private IChangeToken _changeToken = null;
        private CancellationTokenSource _cancellationTokenSource = null;

        private IReadOnlyList<Endpoint> CreateEndpoints()
        {
            var endpoints = new List<RouteEndpoint>();

            for (int i = 0; i < 3; i++)
            {
                var id = Guid.NewGuid().ToString("N")[0..6];
                endpoints.Add(new RouteEndpoint(
                    requestDelegate: DisplayCurrent,
                    routePattern: RoutePatternFactory.Parse($"/{id}"),
                    order: 0,
                    metadata: new EndpointMetadataCollection(new HttpMethodMetadata(new[] { "GET" })),
                    displayName: id));
            }

            return endpoints;
        }

        public void Cancel()
        {
            var old = _cancellationTokenSource;
            _cancellationTokenSource = null;
            old?.Cancel();
        }

        private void EnsureEndpoints()
        {
            if (_endpoints == null || _cancellationTokenSource == null || _changeToken == null)
            {
                lock (_locker)
                {
                    if (_endpoints == null || _cancellationTokenSource == null || _changeToken == null)
                    {
                        _endpoints = CreateEndpoints();
                        _cancellationTokenSource = new CancellationTokenSource();
                        _changeToken = new CancellationChangeToken(_cancellationTokenSource.Token);
                    }
                }
            }
        }

        public override IReadOnlyList<Endpoint> Endpoints
        {
            get
            {
                EnsureEndpoints();
                return _endpoints;
            }
        }

        public override IChangeToken GetChangeToken()
        {
            EnsureEndpoints();
            return _changeToken;
        }
    }
}

In MVC projects, IActionDescriptorChangeProvider may be used to do the my.Cancel() operation.

[yang-er]

After launching this project, you can see the n of - / HTTP: GET (total n metadatas) increasing while refreshing the page /.

And at the beginning, the current endpoint has only one Microsoft.AspNetCore.Routing.HttpMethodMetadata.

After visiting /cancel, you can see there's several same HttpMethodMetadata in the metadata collection of the homepage endpoint.

If you don't visit the homepage (which means, no calls to ModelEndpointDataSource.Endpoints) but /cancel several times, you can also see the duplicate HttpMethodMetadata.

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.