Multiple requests at startup cause ActionSelector to run multiple times

[kyschouv]

I'm trying to debug an issue with the versioning middleware in our project. It seems that if I make more than one request at a time at service startup, the service somehow gets into a state where it runs the action selector twice for the request without clearing between (which causes it to fail because the same action always gets added twice to the ApiVersionProperties().SelectionResult, which fails). If I make one request upon startup, wait for it to return, then continue making requests, everything works fine no matter how much traffic I hit the service with (it still runs the selector twice, but seems to work fine).

I've tried building a simple repro of this, but it's doesn't seem to happen in a simple service. I'm unfortunately unable to share the code I'm working on. I'm wondering if I might be able to get some guidance for debugging this? Or some ideas for what might be causing it? I've spent a couple days trying to track this down already and don't feel like I've made any progress.

[commonsensesoftware]

I'm assuming you must be using 1.1.0-rc1. Unfortunately, there is a regression that I'm tracking in #133. Coincidentally, any IActionSelector may be called multiple times by ASP.NET Core, which I was able to confirm with the ASP.NET team. That was not a behavior I was expecting. As result, I had to change some implementation details in order to resolve a few other related issues.

For example, in all versions prior to the RC, the following would not work:

• api/resources/{id}?api-version=1.0
• api/resources/{id:int}?api-version=2.0

In V1, the {id} was a string, but in V2 it has been constrained to an integer. Even though both routes are logically the same thing, the string parsing and built-in route tree treat the {id} and {id:int} as different candidates. This scenario will cause the IActionSelector to get called multiple times for - basically - the same route.

I was able to address this problem in the RC, but I regressed an older scenario that wasn't caught in my test suite. A similar, but different issue arises where two routes are ambiguous:

• api/resources/{id}?api-version=1.0
• api/resources/search?api-version=1.0

Since all candidates need to be considered, {id} and search are ambiguous because search as text can be provided for the value {id}. The default IActionSelector doesn't have this problem because the route tree parses search and {id} separately and matches search first.

I'm close to having a fix. What needs to happen is to remember which candidates where matched per action selection pass. For there to be successful match, there needs to be exactly one match at any particular action selection iteration. This behavior will allow search to be ranked higher and as a better candidate than {id}.

Hopefully you don't go cross-eyed thinking about all that. I know I certainly have. I'm going as fast as I can to fix this and not regress anything again. I hope to have something published in the next 1-2 days. Thanks for your patience.

[kyschouv]

Thanks for the speedy response. I've certainly dug a lot deeper than I'd intended.

Definitely am on 1.1.0-rc1. However, I'm experiencing this issue on a route with no conflicts:

[ApiVersion("0.7")]
[Route("api/v{version:apiVersion}/users")]
public class UsersController : Controller
{
    [HttpGet("me")]
    public async Task<string> GetMe()
    {
        ...
    }
}

We do have another version 0.8 in our project. Could it be having trouble matching versions?

I'm also seeing an extra one of these in the stack trace on the second call:

    [0HL5313891P66] Microsoft.AspNetCore.Builder.RouterMiddleware+<Invoke>d__4.MoveNext
    [0HL5313891P66] System.Runtime.CompilerServices.AsyncTaskMethodBuilder.Start
    [0HL5313891P66] Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke

If you'd like me to test an update before publishing let me know - I'm happy to dogfood. I'll be somewhat blocked until then anyway.

[kyschouv]

Some more info:

The ActionDescriptors have different Ids, which prevents me from building a workaround as well (at least easily, since I was hoping I could just not add them to the selectionResult.MatchingActions collection if the action id already existed).

[kyschouv]

I tried out 5a78948, and it seems to resolve this issue. However, I'm still experiencing a failure in our unit tests (that worked before).

Controller:

    [ApiVersion("1.0")]
    [ApiVersion("1.1")]
    [ApiVersion("1.1-alpha")]
    [Route("api/[controller]")]
    [Route("api/v{version:apiVersion}/[controller]")]
    public class PathVersioningController : Controller
    {
        [HttpGet("hello")]
        public async Task<object> Hello()
        {
            return await Task.FromResult("v1.0");
        }

        [HttpGet("hello"), MapToApiVersion("1.1")]
        public async Task<object> HelloV1_1()
        {
            return await Task.FromResult("v1.1");
        }

        [HttpGet("hello"), MapToApiVersion("1.1-alpha")]
        public async Task<object> HelloV1_1_Alpha()
        {
            return await Task.FromResult("v1.1-alpha");
        }
    }

Tests:

[Fact]
        public async Task CallOriginalHelloApi()
        {
            var server = CreateServer();
            var response = await server.CreateClient().GetAsync("/api/v1.0/pathversioning/hello");
            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
            Assert.Equal("v1.0", response.Content.ReadAsStringAsync().Result);
        }

        [Fact]
        public async Task CallHelloApiV11()
        {
            var server = CreateServer();
            var response = await server.CreateClient().GetAsync("/api/v1.1/pathversioning/hello");
            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
            Assert.Equal("v1.1", response.Content.ReadAsStringAsync().Result);
        }

        [Fact]
        public async Task CallHelloApiWithWrongVersion()
        {
            var server = CreateServer();
            var response = await server.CreateClient().GetAsync("/api/v9.9/pathversioning/hello");
            Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
            Assert.Contains("UnsupportedApiVersion", response.Content.ReadAsStringAsync().Result);
        }

        [Fact]
        public async Task CallHelloApiNoVersionSpecified()
        {
            var server = CreateServer();
            var response = await server.CreateClient().GetAsync("/api/pathversioning/hello");
            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
            Assert.Equal("v1.0", response.Content.ReadAsStringAsync().Result);
        }

        [Fact]
        public void ApiVersionString()
        {
            var apiVersion = new ApiVersion(1, 0, "alpha");
            Assert.Equal("1.0-alpha", apiVersion.ToString());
        }

        [Fact]
        public async Task CallHelloApiV11Alpha()
        {
            var server = CreateServer();
            var response = await server.CreateClient().GetAsync("/api/v1.1-alpha/pathversioning/hello");
            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
            Assert.Equal("v1.1-alpha", response.Content.ReadAsStringAsync().Result);
        }

        private static TestServer CreateServer()
        {
            var builder = new WebHostBuilder()
                .Configure(app =>
                {
                    app.UseMvc();
                })
                .ConfigureServices(services =>
                {
                    services.AddApiVersioning(options =>
                    {
                        options.AssumeDefaultVersionWhenUnspecified = true;
                        options.DefaultApiVersion = new ApiVersion(1, 0);
                    });
                    services.AddMvcWithBestPractices();
                });

            return new TestServer(builder);
        }

[commonsensesoftware]

Thank you! You rock!

I believe the reason your test isn't working is due to the breaking change in the RC. I can't list the line number, but you need to change your configuration to this in CreateServer:

.Configure(app =>
{
    app.UseMvc();
    app.UseApiVersioning(); // you need this in the RC+
})

If you're interested, you can follow my tests for an example of the setup in the AcceptanceTest.cs. The DefaultApiVersion is already "1.0" so you don't need to configure that unless you really want to be explicit about it (nothing wrong with that). In addition, ApiVersion.Default also equals "1.0".

Your confirmation nearly ensures the issue has been correctly resolved. Thanks again.

[kyschouv]

Oh right, I had fixed that in our other test (and it resolved the issue) but completely missed it here.

All my unit tests run successfully against that changelist now (and my load tests seem to be back to failing on my own bugs =P).

Any ETA on a new version publish to nuget? I won't be able to commit to our repo until I have a package version others will be able to restore (I could hack it in with our nuget.config, but I'd rather avoid that if possible).

[commonsensesoftware]

One more tiny optimization in setting the best match for the open PR. You should see that pushed to that branch within the hour. With those changes in place and your verification, I think happy promoting the change to "RC2". I should be able to get that out by this evening.

[kyschouv]

a6b8122 seems to work as well (unit tests pass, and load test errors seem to all be unrelated to versioning).

[commonsensesoftware]

RC2 has been officially published. Let me know if you run into any other issues. Thanks for you help.