Skip to content
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

StackOverflowException even with MaxDepth of 1 #1398

Closed
cryo75 opened this issue Jun 29, 2016 · 48 comments
Closed

StackOverflowException even with MaxDepth of 1 #1398

cryo75 opened this issue Jun 29, 2016 · 48 comments

Comments

@cryo75
Copy link

cryo75 commented Jun 29, 2016

After upgrading to v5.0.0 from 4.2.1 I'm getting a StackOverflowException when mapping the following model classes to their dto counterparts:

public class UserModel
{
    public virtual UserGroupModel Group { get; set; }
}

public class UserGroupModel
{
    public virtual ICollection<UserModel> Users { get; set; }
}

The mapping configuration is:

cfg.CreateMap<UserModel, UserDto>(MemberList.Destination).MaxDepth(1).ReverseMap();
cfg.CreateMap<UserGroupModel, UserGroupDto>(MemberList.Destination).MaxDepth(1).ReverseMap();

Why am I still getting the exception if MaxDepth is 1?

@lbargaoanu
Copy link
Member

Try the MyGet buld. If it still doesn't work, make a gist that we can execute and see fail.

@cryo75
Copy link
Author

cryo75 commented Jun 29, 2016

The MyGet build didn't work either.

Here is the gist

@lbargaoanu
Copy link
Member

I don't know what's the issue, but the gist runs ok for me. On .NET desktop, right :)? There is also a PR with a similar test passing.

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

When are you getting this exception? Map? ProjectTo? Config time?

On Wednesday, June 29, 2016, Lucian Bargaoanu notifications@github.com
wrote:

I don't know what's the issue, but the gist runs ok for me. On .NET
desktop, right :)? There is also a PR
#1399 with a similar test
passing.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1398 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAGYMr9FXGc2eUUMoycfpOS2vSro3l_Bks5qQmBugaJpZM4JA-OD
.

@khorvat
Copy link

khorvat commented Jun 29, 2016

I have a similar issue, but it only happens when I turn on the CreateMissingTypeMaps.

I have tried to use the CreateMissingTypeMaps on main configuration and on profiles (tested separately) and both. Every time I get StackOverflowException.

I have upgraded from 3.2.1, large app, and I'm getting exception when I try to Map.

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

When do you get this exception?

On Wednesday, June 29, 2016, khorvat notifications@github.com wrote:

I have a similar issue, but it only happens when I turn on the
CreateMissingTypeMaps.

I have tried to use the CreateMissingTypeMaps on main configuration and on
profiles (tested separately) and both. Every time I get
StackOverflowException.

I have upgraded from 3.2.1, large app, and I'm getting exception when I
try to Map.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1398 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAGYMvZS_DSx9KVPdbnz6WvDtGR6nqtLks5qQmI1gaJpZM4JA-OD
.

@cryo75
Copy link
Author

cryo75 commented Jun 29, 2016

@jbogard The exception occurs on Map.

@khorvat
Copy link

khorvat commented Jun 29, 2016

Same here, as I mentioned.

@cryo75
Copy link
Author

cryo75 commented Jun 29, 2016

@jbogard @lbargaoanu I've reverted back to 4.2.1 (but keeping the same configuration setup as for 5.0.0). It works fine with or without using MaxDepth.

So the problem must be in 5.0.0.

@khorvat
Copy link

khorvat commented Jun 29, 2016

@cryo75 do you have CreateMissingTypeMaps turned on ? Just to test few more things and be sure that we are talking about the same issue here ?

@lbargaoanu
Copy link
Member

We still need a repro. About CreateMissingTypeMaps, that can cause a lot of issues, so if you know the types at compile time, just don't use it.

@cryo75
Copy link
Author

cryo75 commented Jun 29, 2016

@khorvat It works both when "CreateMissingTypeMaps" is on or off. My issue is that when calling "Map" on an object that has reference looping, v5.0.0 throws a "StackOverflowException" but 4.2.1 does not.

As a test, you should try downgrading to 4.2.1 and test it out.

@khorvat
Copy link

khorvat commented Jun 29, 2016

@lbargaoanu well I just upgraded a large app from 3.2.1 that has few thousands types, scattered across modules etc. In 3.2.1 I had lazy CreateMap custom functionality that was automatically creating maps for me. After the upgrade I had to remove that auto create map functionality and now I don't have many CreateMap calls. I tried to use CreateMissingTypeMaps and I get StackOverflowException, I have created configuration and all profiles are loaded, when I hit first Map I get the StackOverflowException.

@cryo75 I'll try to downgrade to 4.2.1 and test it.

@lbargaoanu
Copy link
Member

@khorvat You can use the non generic version of CreateMap and create the needed maps with some reflection code.

@khorvat
Copy link

khorvat commented Jun 29, 2016

@lbargaoanu I had that, but I was hoping that I can remove my code and use _CreateMissingTypeMaps _ to do that for me in 4.x or 5.x version

@lbargaoanu
Copy link
Member

What AutoMapper wants is not what it needs :) That's why CreateMissingTypeMaps is not that useful. It starts creating all the maps it thinks it needs, but many times that doesn't help. It's just a different problem.

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

No no no, CreateMissingTypeMaps is when you don't know the types at compile time, nothing else. It is not a shortcut to not call CreateMap. Remove that CreateMap and now you have no idea if something doesn't match up, it just silently skips it. If you know the types you should be using CreateMap and AssertConfigurationIsValid otherwise you're in for a world of pain.

@khorvat
Copy link

khorvat commented Jun 29, 2016

@jbogard ok I see, then I'll try my luck with 5.x and try to kick in my old code for auto CreateMap, thanks.

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

What was your code for "auto CreateMap"? Was it using CreateMissingTypeMaps or no?

Conventionally creating maps is OK, CreateMissingTypeMaps waits until execution time to create a map. The real value is AssertConfigurationIsValid.

@khorvat
Copy link

khorvat commented Jun 29, 2016

@jbogard no it wasn't using CreateMissingTypeMaps it had our bizz logic to scan the objects and execute CreateMap, lazy when Map is called by checking for TypeMaps. Now in 4.x and 5.x architecture is different so I have to rewrite the code.

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

That's OK to do that then. The only difference is in 5.0 instead of calling Mapper.CreateMap, you'll create a MapperConfigurationExpression and call CreateMap on that. Carry on!

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

To be explicit:

var cfgExpr = new MapperConfigurationExpression();
cfgExpr.CreateMap<Source, Dest>();

BizzLogic.Scan(cfgExpr);

var config = new MapperConfiguration(cfgExpr);
IMapper mapper = new Mapper(config);

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

@cryo75 what's the difference between your gist that fails and the PR that is currently passing?

@cryo75
Copy link
Author

cryo75 commented Jun 29, 2016

@jbogard The difference between my actual code and the gist that I did (which is similar to the PR) is that both my models are much more complex than just a property each. However, it is these two particular properties which result in circular referencing.

And as I said, 4.2.1 works fine.

@TylerCarlson1
Copy link
Member

@khorvat what you should be looking at is Conventions to replace your lazy CreateMap code not CreateMissingMaps property. The convention will give you both source type and dest type, so from there you can add your logic to scan the types and see if it should create a new TypeMap or not.

Unfortunately this probably won't solve the MaxDepth StackOverflowException problem. It's just the recommended way of adding maps based on user set criteria.

@cryo75
Copy link
Author

cryo75 commented Jun 29, 2016

@jbogard Ok it turns out that it was a completely different member that's causing circular referencing. I've updated the gist to reflect this (and will hopefully make the PR fail).

In my actual code, using v5.0.0 and setting MaxDepth to 1 doesn't cause the exception. However, using v4.2.1 and no MaxDepth will not cause the exception either.

However, setting the MaxDepth to 1 (in both versions) does not map the misbehaving property. ie. as per the gist, "UserDto.Category" remains null.

@lbargaoanu
Copy link
Member

In 5.0 PreserveReference is false by default (used to be true) and that's probably the root cause. Nothing to do here.

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

@lbargaoanu I wonder if a better exception message would be appropriate? Catch a StackOverflow and add a better message?

@lbargaoanu
Copy link
Member

There are exceptions that you catch and exceptions that you don't. SO is in the latter category :). But seriously, you're out of stack, there isn't much you can do reliably by then. But I see your point, it's frustrating to just crash. Maybe to detect it before it happens, but at zero cost. I guess it's not going to happen.

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

@cryo75 can you try PreserveReferences? That was another insane perf killer I had to default to false.

@jbogard
Copy link
Member

jbogard commented Jun 29, 2016

@lbargaoanu yeah you're right, just thinking through if there were a way to detect this for people at config time.

@cryo75
Copy link
Author

cryo75 commented Jun 30, 2016

@jbogard PreserveReferences doesn't affect the mapping. The member remains null. I guess the only solution is to rethink the model so I can keep on using 5.0.0.

@khorvat
Copy link

khorvat commented Jun 30, 2016

@jbogard I have tried the suggestion from @TylerCarlson1 about using AddConditionalObjectMapper and added the code below to my configuration (same as on wiki), just to see if this is the right path to go, now I get _StackOverflowException _ same situation as @cryo75 and his gist.

What is the preferred solution here ? Rethink the model or something else ? In this part of the app my models are generated by the Entity Framework and the props causing this issue are navigation props, so I'm not sure I can rethink the whole model here.

var config = new MapperConfiguration(cfg => {
    cfg.AddConditionalObjectMapper().Where((s, d) => s.Name == d.Name + "Poco");
});

@khorvat
Copy link

khorvat commented Jun 30, 2016

BTW I have just downgraded to 4.2.1 and almost everything is working, as I have removed my auto create map logic (didn't put it back yet), I tried to turn on CreateMissingTypeMaps and test, now TypeMaps are created automatically and there is no StackOverflowException killing the app.

Will try 4.2.1 for a while and upgrade to 5.0 as soon as this issue if fixed. Eager to get 5.0 performance boost.

@lbargaoanu
Copy link
Member

There will not be any fix without a repro.

@khorvat
Copy link

khorvat commented Jun 30, 2016

Did @cryo75 's gist reproduced the issue ?

@jbogard
Copy link
Member

jbogard commented Jun 30, 2016

Nope :(

On Thursday, June 30, 2016, khorvat notifications@github.com wrote:

Did @cryo75 https://github.com/cryo75 's gist
https://gist.github.com/cryo75/e4699b021d26820203e6896bd6532962
reproduced the issue ?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1398 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAGYMsWDq7_Ouso3m7aYruLtIEGKmnetks5qQ6_OgaJpZM4JA-OD
.

@khorvat
Copy link

khorvat commented Jun 30, 2016

Ok, will try to provide repro gist

@lbargaoanu
Copy link
Member

And try PreserveReferences. Because that's not a bug, we wanted to change that one :)

@khorvat
Copy link

khorvat commented Jun 30, 2016

Here is the gist AutoMapper StackOverflowException issue related to CreateMissingTypeMaps and AddConditionalObjectMapper

In both cases I get StackOverflowException while this is working in 4.2.1

I don't know how to turn on PreserveReferences on dynamically created TypeMaps when using CreateMissingTypeMaps or AddConditionalObjectMapper any suggestions ?

@TylerCarlson1
Copy link
Member

Can you try cfg.AllowNullDestinationValues = false; and see if that helps. Have similar issue #1352, but it's calling ProjectTo() with circular dependency that causes the StackOverflowException.

Also can you give a complete exception message with stack trace, that will help determine the exact source of the problem.

@jbogard
Copy link
Member

jbogard commented Jun 30, 2016

@jbogard
Copy link
Member

jbogard commented Jun 30, 2016

One thing we could look at is to detect circular references and throw during configuration validation.

@khorvat
Copy link

khorvat commented Jun 30, 2016

@jbogard is there a way to set PreserveReferences when using CreateMissingTypeMaps or AddConditionalObjectMapper ?

@TylerCarlson1
Copy link
Member

@khorvat Currently no. It creates a new TypeMap and follows the conventions of the Profile. There's nothing in the Profile that has the ability to get the TypeMap when a new TypeMap is generated. This was a possible feature for Conventions, but since we didn't know what it might be used for we didn't add it. If you want you can make a new Issue to request for modifying TypeMap after it's generated by AddConditionalObjectMapper/CreateMissingTypeMaps

@TylerCarlson1
Copy link
Member

@khorvat Sorry I think miss-spoken. cfg.ForAllMaps() should work on CreateMissingTypeMaps and AddConditionalObjectMapper. I thought it only worked on TypeMaps defined in the configuration before.

@khorvat
Copy link

khorvat commented Jul 1, 2016

@TylerCarlson1 I have opened a separate issue #1422 thanks.

@lock
Copy link

lock bot commented May 7, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants