Issue when YARP is combined into a "normal" app

[glatzert]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

After moving to .NET7, I found a problem, that occures, when YARP is part of the application.
Since it seems to be somewhere between YARP, ASP.Net core and .NET7, I like to bring it to your attention here.

It makes using NET7 for use-cases with BFF driven by YARP "difficult" and ends up in lots of these:

AmbiguousMatchException: The request matched multiple endpoints. Matches:

/Index
/Index
/Index

Heres the Issue with repro dotnet/yarp#1864

Expected Behavior

I'd expext YARP not to break by upgrading from .NET6 to .NET7

Steps To Reproduce

https://github.com/glatzert/YARPBug

Exceptions (if any)

No response

.NET Version

7.0.100-rc.1.22431.12

Anything else?

No response

[Tratcher]

There's no conflicting routes in the app. Yarp is on "api/{**catch-all}" and the razor pages are at the root.

@halter73 can you check what's going on with the host's implicit route builder and why adding yarp might trigger double razor registrations?

[glatzert]

I'll add, it's not only problematic with Razor pages, but controllers will also show the AmbigousMatchException behavior

[davidfowl]

@halter73 it seems like the CompositeEndpointDataSource changes are what's making it add duplicates. I'll make a minimal repro to show the issue. I understand it now.

[davidfowl]

using Microsoft.Extensions.Primitives;

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

((IEndpointRouteBuilder)app).DataSources.Add(new CustomDS());

app.MapGet("/", () => "Hello World!");


app.Run();

class CustomDS : EndpointDataSource
{
    private List<Endpoint>? _endpoints;
    private CancellationTokenSource _cts = new();

    public override IReadOnlyList<Endpoint> Endpoints
    {
        get
        {
            if (_endpoints is null)
            {
                _endpoints ??= new();

                var old = _cts;
                _cts = new();
                old.Cancel();
            }

            return _endpoints;
        }
    }

    public override IChangeToken GetChangeToken()
    {
        return new CancellationChangeToken(_cts.Token);
    }
}

Firing the change token after producing endpoints is causing the composite data source to re-evaluate the list while it's populating it (a side effect of accessing endpoints).

[halter73]

This was partially mitigated by #43729. _endpoints is still null the first time GetChangeToken() fires, so we avoid stack diving the first time endpoints are resolved in this case.

However, if you update the sample to always trigger the change token in Endpoints and to resolve the root enpoints more than once, it can still stack overflow. I'm not sure how common this is, but it would be easy to avoid reloading endpoints if we're already in the middle of doing so on the same thread up stack.

using Microsoft.Extensions.Primitives;

var app = WebApplication.Create(args);

var customDS = new CustomDS();
((IEndpointRouteBuilder)app).DataSources.Add(customDS);
// Force evaluation of the composite data source endpoints. The same could be done with an HTTP request, but this is easy.
_ = app.Services.GetRequiredService<EndpointDataSource>().Endpoints;
// Now that _enpoints is initialized. Trigger the change token to induce a stack overflow.
customDS.Refresh();

// We don't get this far.
app.Run();

class CustomDS : EndpointDataSource
{
    private List<Endpoint>? _endpoints;
    private CancellationTokenSource _cts = new();

    public void Refresh()
    {
        var old = _cts;
        _cts = new();
        old.Cancel();
    }

    public override IReadOnlyList<Endpoint> Endpoints
    {
        get
        {
            _endpoints ??= new();

            var old = _cts;
            _cts = new();
            old.Cancel();

            return _endpoints;
        }
    }

    public override IChangeToken GetChangeToken()
    {
        return new CancellationChangeToken(_cts.Token);
    }
}

Previously, the CompositeEndpointDataSource would unregister from change tokens while evaluating endpoints, but if someone were to add another endpoint on another thread while this was happening, we would miss the update.

[halter73]

I'm currently doing hackathon things, but I think this is our best bet to fully fix it without introducing race conditions where we might miss changes originating from another thread.

It uses a [ThreadStatic] Environment.CurrentManagedThreadId ThreadLocal<bool> to track if a child EndpointDataSource is triggering a change token inline when the CompositeDataSource is reading from the child Endpoints or resolving a new change token without invoking user change registration with a lock or risking missing changes originating in parallel on another thread. If we agree with this approach, I can add a test and open a PR.

Given that this is partially mitigated, do we still want to try to fix this for .NET 7? Or do we just want to tell developers to stop raising change events when endpoints are merely being accessed not changed?

[Tratcher]

Who is raising the change event in this case? YARP? MVC?

[davidfowl]

https://github.com/microsoft/reverse-proxy/blob/5a86eb4f18474bbe2f9e73f8457f4cbf13c00b81/src/ReverseProxy/Management/ProxyConfigManager.cs#L141

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.

[rafikiassumani-msft]

@glatzert This may be fixed in rc2. Can you install a .NET rc2 nightly build and let us know if the issue is fixed on your end ?
https://github.com/dotnet/installer