Skip to content

Getting sometimes AmbiguousActionException with only 1 matching action #248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pgrm opened this issue Feb 8, 2018 · 11 comments
Closed

Getting sometimes AmbiguousActionException with only 1 matching action #248

pgrm opened this issue Feb 8, 2018 · 11 comments

Comments

@pgrm
Copy link

pgrm commented Feb 8, 2018

On our Jenkins build agent we are also stress testing our server a little bit, and every hundredth build or so fails with an error similar to

...|Microsoft.AspNetCore.Mvc.Routing.DefaultApiVersionRoutePolicy|ERROR|Request matched multiple actions resulting in ambiguity. Matching actions:
OUR_NAMESPACE.Controllers.InventoryController.CheckInventoryAsync (OUR_MODULE)

the odd thing, it is always just one matching action, and as I mentioned before, it happens very irregularly. So far, the issue could always be resolved, by restarting the build.

We looked into it, and saw that before the error is created, only the distinct matched actions are being selected: https://github.com/Microsoft/aspnet-api-versioning/blob/0ec42dc0685f7d8292bcbda202498c77591cae02/src/Microsoft.AspNetCore.Mvc.Versioning/Routing/DefaultApiVersionRoutePolicy.cs#L183

Our best guess so far is, that for some weird reason, sometimes the same action is registered multiple times. This would explain why the error can't really be reproduced, and why the error message shows only one action.

@commonsensesoftware
Copy link
Collaborator

Odd. There have been one or two similarly bizarre errors reported related to the same area. I'll look into this more and see if there isn't something that can be hardened. These types of issues are notoriously very difficult to track down and resolve. I suspect I can tighten a few things up, but it will be difficult to know I've solved the problem because it's so difficult to reproduce.

I'll see what I can do. Thanks for reporting the issue.

@pgrm
Copy link
Author

pgrm commented Mar 14, 2018

@commonsensesoftware FYI we just got the error also on production, but only on one endpoint - redeploying fixed it - anything we can do to help finding the source?

@commonsensesoftware
Copy link
Collaborator

Once it goes into a bad state, does it stay that way? I'm at a bit of loss, but I'll reach out to the ASP.NET team and see if there is a way to capture what's happening.

Perhaps there's a flaw with ActionDescriptorMatch.cs. The assumption is that only a single instance of ActionDescriptor is created per controller and action. There may be a scenario (perhaps under pressure?) where the infrastructure creates two instances of an ActionDescriptor for the same action. This should lead to a scenario where there are duplicate matches, but only a single matching action is reported. Should that be the case I'm not exactly sure how to group matches together.

I suspect this might be the culprit.

@commonsensesoftware
Copy link
Collaborator

Small update. I actually just ran into an unrelated setup that resulted in the symptoms you are describing. The cause was truly from duplicate routes; however, the action names were the same. This gave the appearance that there were two matching actions with the same name.

I'm not sure that this is your scenario, but this is what it would look like:

public IActionResult Get() => Ok();

public IActionResult Get(int id) => Ok();

Where these are defined on the same controller type or different types, if there is a duplicate match, the error message will use the display name "Get" for both actions. This will yield one name with multiple actions.

I'm not sure that this is, in fact, your scenario, but it could be - somehow. The error message needs to be improved in order to confirm that the actions are different and exactly which two actions we're talking about.

I can't explain why this only happens occasionally. I would expect this behavior to happen a lot. Regardless, there isn't currently enough information to troubleshoot any potential conflict. I'll add some logging around this area and let you know when the package is out.

@pgrm
Copy link
Author

pgrm commented Mar 26, 2018

@commonsensesoftware thank you - I was also thinking at first about the issue you described, and these are methods we have for instance:

  • GET /
  • GET /{id}

and we also have

  • for V1 - GET /
  • for V2 - GET /

both of those could be in the same controller. From my side those are 2 different actions, since they either have a different URL or a different version annotation.

Anyhow, let me know when there will be an update package with more logs available and I'll update our project and let you know as soon as it happened again. Although since my last message 12 days ago, when I panicked slightly, it stopped occurring. Maybe we got rid of some old version endpoints on the API. Will be checking that more closely in the future. Thx for your support 👍

@pgrm
Copy link
Author

pgrm commented Mar 28, 2018

@commonsensesoftware I just got another issue, which seems to me that it might be related, this time it was a 400 error reported by ASP.NET Core:

{
  "error": {
    "code": "UnsupportedApiVersion",
    "message":
      "The HTTP resource that matches the request URI 'URI_HERE' does not support the API version '2'."
  }
}

I just checked and the controller in question is annotated with the attributes V1 and V2, those are custom attributes which derive from ApiVersionAttribute:

[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = false)]
public sealed class V1Attribute : ApiVersionAttribute
{
    public V1Attribute()
        : base(Constants.Versions.VersionOne)
    {
    }
}

This means, the controller is available in exactly the same structure in V1 and V2, there are no differences, and each method ...

  • get list GET /v2/controller
  • get item GET /v2/controller/{itemId}
  • add new item POST /v2/controller
  • update item PATCH /v2/controller/{itemId}

is implemented only once and used for V2 as well as V1.

I restarted the build and it worked again, like before.

@commonsensesoftware
Copy link
Collaborator

Hmm... still not sure how that could happen. Your extension is valid. The model conventions would see this pretty early in the pipeline.

I've published a new version of the package with the latest round of fixes. I also included additional information in the logging so that the fully-qualified method name will be displayed with parameter signature. That should help with the ambiguous scenario. I'm not sure sure about this new 400 issue.

I'm hoping to start on a beta 3.0 branch this next week. I'm going to revisit removing the need for a custom router at the end of the pipeline. Some routing changes that happened in ASP.NET 2.0+ may have made that unnecessary now. This would solve a bunch of issues folks are facing with middleware and should elimintate some of the oddies being observed - like here.

@commonsensesoftware
Copy link
Collaborator

Circling back around on this. An internal team at Microsoft discovered an issue where the runtime modification to a dictionary can cause CPU utilization to hit 100% in certain scenarios. That's not the symptoms you described, but there are many similarities. The team provided a full ETW trace of the offending call stack and the issue happens at almost the exact same place that you have been seeing.

I've done some refactoring that will pre-compute the dictionary content and completely mitigate the issue. I suspect this may solve your issue once and for all too. You can find the latest package with this fix in v2.3.0. If or when you have bandwidth to report back whether you stopped observing this issue, that would help confirm this issue is, in fact, resolved.

Thanks

@pgrm
Copy link
Author

pgrm commented Jun 14, 2018

hey @commonsensesoftware - thx for the fixes

I've just updated our application and removed a workaround with which we tried to decrease the likely hood of the error. I'll get back to you in couple of days to close this issue (hopefully) if we didn't experience that error.

Thank as ton for the support

@commonsensesoftware
Copy link
Collaborator

Did this issue finally go away?

@pgrm
Copy link
Author

pgrm commented Jul 15, 2018

@commonsensesoftware yes, seems to be gone, thank you!

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

No branches or pull requests

2 participants