HttpPost with entity as parameter stuck in an infinite loop? (rc2-final)

[HairyPortal]

Consider a call to a simple post action like this

    [HttpPost]
    public async Task<IActionResult> Create(Organization data)
    {
        return NotFound();
    }

The call is unsuccessful and cause memory consumption go up wildly (1 GB every 30 second) and it never stop and consume all my memory.

It looks like the call is stuck in an infinite loop of some sort.

After some experiment, I found out that only classes that I use in DbContext cause this infinite loop. Swapping "Organization" (which is a class I used in DbContext) with other class like the one I used as viewmodel dose not cause this.

I'm not sure what I did wrong because in rc1 everything works just fine.

[xrkolovos]

I am having the same issue. I have a complex ef model, and i get as parameter in the post method one object of this model. It's like the framework trying to discover all the dependancies with this model and iterates through the whole model.

i am running on net46

[xrkolovos]

i am testing this bug for like 4 hours. After adding more complexity(i mean more objects with more connections between them) the time that takes to execute the method rises. At a given number of objects (with connections between them), the app breaks and it nevers hit the method inside the mvc controller. After that also we get the memory leak.

The number of the classes in my model is only 24. Houston, We've Got a Problem.

[fglebel]

I am also stuck in a loop. Unable to advance. Was working fine in RC1. Unable to hit breakpoint.

I am using EFCore. Did a database first scaffold.

[Bartmax]

Just in case, a word from the MS team will be great.

[rynowak]

Just in case, a word from the MS team will be great.

Could one of you experiencing the issue grab a call-stack of where MVC is spending most of its time?

[xrkolovos]

can you provide some info how to get one? Because i don't know how. After i am skipping the last line of the constuctor the stact trace window is empty.

[xrkolovos]

you only need an complex model with about 20-25 classes and relations between them. if you need one, i can created for you.

[fglebel]

Since I cannot hit the breakpoint, call stack is empty. I am trying to reproduce from scratch.

[xrkolovos]

@fglebel how big is your model? (how many classes do you have related)

[xrkolovos]

I created a sample project that demonstrate this bug in OrderNoteController.cs with 2 methods. One with a complex class and one with a Viewmodel Class.

I used the domain model of this project https://github.com/smartstoreag/SmartStoreNET/tree/4db9e2f6f24eea13402fa869a5d88f5b20ee6c52/src/Libraries/SmartStore.Core/Domain

Decisions.WebCoreUi.zip

[fglebel]

I was able to reproduce quite easily.

• I created a new ASP.NEW Core Web Application (.NET Core) web application with Windows Authentication from VS Studio 2015 Update 2.
• Installed EF Core
• Did a dotnet ef dbcontext scaffold of my SqlServer database into a Models folder
• Added 1 index view, 1 edit view and 1 controller

startup.cs for services: added a dbcontext added an authorize policy

Nothing changed inside app

project.json added dependencies: "Microsoft.EntityFrameworkCore.Tools": "1.0.0-preview1-final", "Microsoft.EntityFrameworkCore.SqlServer": "1.0.0-rc2-final", "Microsoft.EntityFrameworkCore.SqlServer.Design": "1.0.0-rc2-final"

added tools: "Microsoft.EntityFrameworkCore.Tools": { "imports": [ "portable-net451+win8" ], "version": "1.0.0-preview1-final" }

Here are all the references:

[fglebel]

@fglebel how big is your model? (how many classes do you have related)

@xrkolovos I have about 200

[xrkolovos]

Can we hope for a quick fix, in a new AspNetCore.Mvc Package, or a quick solution adding a class that impements a custom binding functionality overwriting the default one?

Any more info or workaround from the team will be helpful because we are stuck, and the next thing to do is migrate back to rc1.

[webeg]

Just in case, a word from the MS team will be great.

Could one of you experiencing the issue grab a call-stack of where MVC is spending most of its time?

@rynowak any news on this one?

[rynowak]

I'm investigating this currently, will update here when I know more

[rynowak]

TLDR

I have a workaround for this, and I'm currently verifying the workaround against our test suite.

For anyone that's interested in trying it out before I've had a change to verify the change in full, here's a preview: https://gist.github.com/rynowak/9488b3b1a6b81876d8b14de72feb0857

To use this, paste that class into your project, and add the following line to your ConfigureServices

services.AddSingleton<IModelBinderFactory, MyModelBinderFactory>();

What went wrong?

We made a change in RC2 to build a 'graph' of model binders up front the first time a parameter is bound. This allows us to do any expensive work necessary to figure out how to bind the first time, and take advantage of caching on each subsequent request.

What's causing you pain is that your model classes are deeply nested, or encounter the same types/properties along different paths through the model graph. The implementation of IModelBinderFactory shipped in 1.0.0-rc2-final doesn't attempt to share any intermediate results.

For a model like this:

public IActionResult CheckOut(Customer customer) { }

class Customer 
{
    int Id { get; set; }
    IList<Product> ProductsInCart { get; set; }
    IList<Product> ProductsViewed { get; set; }
}

class Product
{
  .....
}

We'll create at least 6 IModelBinders before we even look at the properties on Product.

• one for the top level parameter (Customer)
• one for the Id
• one for ProductsInCart
• one for ProductsViewed
• and two for the class Product

This means that each property on Product now gets two binders created. Obviously this can scale to way more ridiculous levels than the example I gave here.

The fix is to share the intermediate results, so we're using a Dictionary<> rather than a "stack". We're now doing memoization instead of just cycle breaking. We'll consider making more changes in this area to minimize the number of binders created in the future, this is a quick workaround.

What you should know

If you have an action that exposes a large model, in particular if you're exposing your EF entities directly to model binding - you are vulnerable to overposting attacks! See more here if you're not familar with the issues: http://odetocode.com/blogs/scott/archive/2012/03/11/complete-guide-to-mass-assignment-in-asp-net-mvc.aspx

Even if your are not allowing EF to update all of your entities when you save state, by exposing a deeply nested model structure, you allow attackers to force model binding to create those extra objects and run validation on them, which can overload your site in extreme cases.

The safest thing to do is always to use view models / binding models

[xrkolovos]

Thanks for you quick workaround. I already tested in our app and is working. It also nice sharing this information about overposting attacks.

We are generaly using vm but this is not always the case. Writing an app with the mvc framework might be a very rapid soultion, for a small internal app (as in my case now). In that case you might now have the time for optimal usage and also you might not have the threads of the internet. In that case, if the model is a litle bit complicated it might be hard to have an up front 'graph' of model with good performance. It might be a good idea this feature to be optional.

[buraktamturk]

Could anyone publish @rynowak's workaround to nuget? I would like to do myself but I just don't know which framework to target (I just target full framework).

[xrkolovos]

you dont need a package from nuget, and this solution is a simple workaround.

To use this, paste that class into your project, and add the following line to your ConfigureServices services.AddSingleton<IModelBinderFactory, MyModelBinderFactory>();

the class -> https://gist.github.com/rynowak/9488b3b1a6b81876d8b14de72feb0857 the "services.AddSingleton.." line in your startup.cs

[buraktamturk]

@xrkolovos well, what I am trying to avoid is copying/maintaining the class to my projects individually. People usually maintain more than one project. And this class is irrelevant the purpose of many projects. (I mean just think mvc/web based calculator project and MyModelBinderFactory.cs just why?)

If someone makes this a class library, we could just import it like "AspNetCore.Mvc.Workaround4666": "1.0.0" then call services.AddSingleton just as usual. So we don't have to bloat our projects.

[Bartmax]

@buraktamturk you can save the file somewhere on your disk and link it on your projects. or create a class library with that file and reference as a dependency using the projects property on global.json if you prefer to have it as a package. I don't see any value of having this as an online nuget package.

[fglebel]

Also working for me. thank you.

[xrkolovos]

Nuget packages mean more permanent solutions, thats not the point of workarounds. But you can always create your personal nuget package and share it accross many projects. But workarounds means that your must change it after.

(MyModelBinderFactory.cs i didn't used this name, i thing you souldn't)

[buraktamturk]

Agreed on a few points but I have a few projects hosted in git, and I don't like copying external contents, since they add complexity. Since it is on git, I can't copy the file once to my computer and link it to another projects.

I published "Tamturk.AspNetCore.Mvc.Workarounds.id4666" to nuget it has "1.0.0-rc2-final" version tag as others. It targets both netstandard1.5 and net46, I don't know whatever it's right thing to target both, since I just use full framework on my projects.

You can just add "Tamturk.AspNetCore.Mvc.Workarounds.id4666": "1.0.0-rc2-final" in dependency section on project.json.

And configure it in Startup like this:

services.AddSingleton<Tamturk.AspNetCore.Mvc.Workarounds.id4666.ModelBinderFactory4666>();

Since It's just workaround for only rc2-final, just remove them for next version of the project.

[prachya]

I've been fixing this problem too. I did it follow the post of @rynowak and the problem fixed - hope an official fix will be available soon.

[rynowak]

Fixed for 1.0.0 78c130d2268c4c011bece7249f47b0f419c66b3d

[Hayder90]

Hello Everyone , I had the same issue with the form post , i followed the instruction of the workaround , now i have a new issue : No parameterless constructor defined for this object when i try to post a form i don't know wich object. i made sure that my model have a parametreless constructure , i ve seen a post talking about the form collection in the IAction result ( if you remove it everything works fine i didn't try this ). my controller have a constructor with parameters thank you

[dougbu]

@Hayder90 suggest opening a new issue with details about your specific problem. Doesn't sound like anything related to #4666.

In the meantime, debugging your application and breaking on exceptions should get the stack where the "No parameterless constructor" issue is hit. That'll narrow down the types involved.

[fhnainia]

Here is the stack from @Hayder90: System.MissingMethodException: No parameterless constructor defined for this object. at System.RuntimeTypeHandle.CreateInstance(RuntimeType type, Boolean publicOnly, Boolean noCheck, Boolean& canBeCached, RuntimeMethodHandleInternal& ctor, Boolean& bNeedSecurityCheck) at System.RuntimeType.CreateInstanceSlow(Boolean publicOnly, Boolean skipCheckThis, Boolean fillCache, StackCrawlMark& stackMark) at System.Activator.CreateInstance(Type type, Boolean nonPublic) at System.Activator.CreateInstance(Type type) at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinder.d3.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Mvc.Internal.ControllerArgumentBinder.d7.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Mvc.Internal.ControllerArgumentBinder.d10.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Mvc.Internal.FilterActionInvoker.d40.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Mvc.Internal.FilterActionInvoker.d39.MoveNext() --- End of stack trace from previous location where exception was thrown --- at Microsoft.AspNetCore.Mvc.Internal.FilterActionInvoker.d32.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Mvc.Internal.MvcRouteHandler.d8.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Builder.RouterMiddleware.d4.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Session.SessionMiddleware.d8.MoveNext() --- End of stack trace from previous location where exception was thrown --- at Microsoft.AspNetCore.Session.SessionMiddleware.d8.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware1.<Invoke>d__18.MoveNext() --- End of stack trace from previous location where exception was thrown --- at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware1.d18.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware`1.d18.MoveNext() --- End of stack trace from previous location where exception was thrown --- at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware1.<Invoke>d__18.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware1.d18.MoveNext() --- End of stack trace from previous location where exception was thrown --- at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware`1.d18.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware1.<Invoke>d__18.MoveNext() --- End of stack trace from previous location where exception was thrown --- at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware1.d18.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.VisualStudio.Web.BrowserLink.Runtime.BrowserLinkMiddleware.d5.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.MigrationsEndPointMiddleware.d5.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DatabaseErrorPageMiddleware.d6.MoveNext() --- End of stack trace from previous location where exception was thrown --- at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DatabaseErrorPageMiddleware.d6.MoveNext() --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.d7.MoveNext()

[fhnainia]

The "No parameterless constructor" issue is fixed in here: #4895

[oyvindvol]

Is this fix already implemented in asp.net core RTM, so that I can delete my MyModelBinder-class from my project now?

[dougbu]

@oyvindvol yes, @rynowak's fix was included in the RTM (1.0.0) release.