Skip to content

CheckForCycles removes the type map from typeMapsPath while its members are still being iterated #4654

Description

@jbogard

Noticed while investigating #4650. This is an observation about the code, not a confirmed bug — I have not produced a repro where it causes wrong mapping output.

What happens

In TypeMapPlanBuilder.CheckForCycles (src/AutoMapper/Execution/TypeMapPlanBuilder.cs:102-160) the path set is maintained as:

typeMapsPath.Add(typeMap);          // :104
foreach (var memberMap in MemberMaps())
{
    ...
    if (memberMap.Inline && (memberTypeMap.PreserveReferences ||
                             typeMapsPath.Count == configuration.MaxExecutionPlanDepth))   // :113-114
    ...
    CheckForCycles(configuration, memberTypeMap, typeMapsPath);   // recursion
}
typeMapsPath.Remove(typeMap);       // :159

The recursive call at the end of each iteration removes its own type map from the shared set before returning. When that recursion visits a map already on the path, the Remove takes it off while the outer frame is still walking that same map's members. Subsequent members in the outer loop then see a shorter typeMapsPath than they should.

Since inlining is decided by typeMapsPath.Count == configuration.MaxExecutionPlanDepth — an exact equality against a default of 2 — a member evaluated after the path has been shortened can miss the depth threshold entirely and stay inlined.

Observed effect

With Order -> Line -> Services where Line is self-referential, whether Line.Services ends up inlined depends on property declaration order on Line:

  • Parent declared first: recursion through Parent re-enters Line, removes it from the path, and Services is then evaluated with Count == 1, so it stays inlined.
  • Services declared first: it is evaluated with Count == 2 and inlining is disabled.

I confirmed the first case by inspecting MemberMap.Inline after a map (Services Inline=True, Parent Inline=False).

Inlining is a performance and plan-shape decision rather than a correctness one, so this may be harmless. But it makes plan shape depend on declaration order and on which root map compiles first, and #4650 showed that order-dependent plan shape is exactly the kind of thing that turns into a non-deterministic production failure when it interacts with something else.

If the intent is that each frame sees the full ancestor path, the recursion needs to restore the set rather than pop a map that an outer frame still owns — or the depth test should not depend on a set being mutated underneath it.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions