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

DryIoc Resolve with decorators goes wrong for parallel execution #116

Closed
notsupported opened this issue Apr 16, 2019 · 6 comments
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@notsupported
Copy link

[Tested on DryIoc 4.0.3 against .NET Framework 4.7.2]

I observed strange behavior of DryIoc Resolve in parallel execution when decorators are being used: At some point the decorator will resolve itself instead of the object it decorates.

Test classes

I have an interface IQuery<TRequest,TResponse> and an implementation of it MyQuery. There's also a decorator QueryDecorator<TRequest, TResponse> implementing the same interface.

public interface IQuery<TRequest, TResponse>
    where TRequest : class
    where TResponse : class
{
}

public class MyQuery : IQuery<MyQuery.Request, MyQuery.Response>
{
    public class Request {}
    public class Response {}
}

public class QueryDecorator<TRequest, TResponse> : IQuery<TRequest, TResponse>
    where TRequest : class
    where TResponse : class
{
    readonly IQuery<TRequest, TResponse> _query;

    public QueryDecorator(IQuery<TRequest, TResponse> query)
    {
        if (query.GetType() == this.GetType())
            throw new Exception("Decorator resolved itself"); // We should never get here

        _query = query;
    }
}

Note the if (query.GetType() == this.GetType()) throw new Exception... in the constructor of the decorator. Obviously that's not something that should be there in production code but it will make the code crash and fail the test further down. The code will only throw an exception if query is resolved to the same type as the decorator itself (which should not happen).

The types are registered as follows:

var container = new Container();
container.Register(typeof(IQuery<MyQuery.Request, MyQuery.Response>), typeof(MyQuery));
container.Register(typeof(IQuery<,>), typeof(QueryDecorator<,>), setup: Setup.Decorator);

Test methods

✅ Sequential execution

First an example that works fine: When resolving sequentially everything works as expected. Don't mind the idiotic task based approach - it's just to have exactly the same code as the parallel example.

[Test]
public async Task DryIoc_Resolve_sequential_execution()
{
    var container = new Container();
    container.Register(typeof(IQuery<MyQuery.Request, MyQuery.Response>), typeof(MyQuery));
    container.Register(typeof(IQuery<,>), typeof(QueryDecorator<,>), setup: Setup.Decorator);

    await Task.Run(() => container.Resolve<IQuery<MyQuery.Request, MyQuery.Response>>());
    await Task.Run(() => container.Resolve<IQuery<MyQuery.Request, MyQuery.Response>>());
}

🔴 Parallel execution

Here's where things go wrong. The test fails and in my opinion it shouldn't.

[Test]
public async Task DryIoc_Resolve_parallel_execution()
{
    var container = new Container();
    container.Register(typeof(IQuery<MyQuery.Request, MyQuery.Response>), typeof(MyQuery));
    container.Register(typeof(IQuery<,>), typeof(QueryDecorator<,>), setup: Setup.Decorator);

    var tasks = new List<Task>
    {
        Task.Run(() => container.Resolve<IQuery<MyQuery.Request, MyQuery.Response>>()),
        Task.Run(() => container.Resolve<IQuery<MyQuery.Request, MyQuery.Response>>())
    };
    await Task.WhenAll(tasks);
}

IMPORTANT: When the parallel execution test is ran together with the other 2 tests, success or failure depends on the order of execution. If the parallel test is not the first, it may actually succeed. So it's best to run this test standalone if you can't reproduce it with the other tests present.

✅ Sequential then parallel execution

If the object is resolved once before, further parallel execution will resolve the object correctly. The test case below executes just fine. So there's no reason to assume the previous test case shouldn't work.

[Test]
public async Task DryIoc_Resolve_sequential_then_parallel_execution()
{
    var container = new Container();
    container.Register(typeof(IQuery<MyQuery.Request, MyQuery.Response>), typeof(MyQuery));
    container.Register(typeof(IQuery<,>), typeof(QueryDecorator<,>), setup: Setup.Decorator);

    await Task.Run(() => container.Resolve<IQuery<MyQuery.Request, MyQuery.Response>>());
    await Task.Run(() => container.Resolve<IQuery<MyQuery.Request, MyQuery.Response>>());

    var tasks = new List<Task>
    {
        Task.Run(() => container.Resolve<IQuery<MyQuery.Request, MyQuery.Response>>()),
        Task.Run(() => container.Resolve<IQuery<MyQuery.Request, MyQuery.Response>>())
    };
    await Task.WhenAll(tasks);
}
@dadhi
Copy link
Owner

dadhi commented Apr 16, 2019

Thanks for the detailed explanation!
No idea what the problem at the moment. Cache is a first culprit. Will check.

@dadhi dadhi self-assigned this Apr 24, 2019
@dadhi dadhi added the bug Something isn't working label Apr 24, 2019
@dadhi dadhi added this to the v4.0.4 milestone Apr 24, 2019
dadhi added a commit that referenced this issue Apr 24, 2019
added: ImTools.Map with additional S state argument to prevent map lambda closure allocations
added: Test for #116
@dadhi
Copy link
Owner

dadhi commented Apr 27, 2019

Found (hopefully) a root cause, related to the cache of generated closed-generic factories. Good opportunity to get rid off this cache.

@dadhi dadhi closed this as completed in ae17163 Apr 29, 2019
Leszek-Kowalski pushed a commit to Leszek-Kowalski/DryIoc that referenced this issue Oct 11, 2019
added: ImTools.Map with additional S state argument to prevent map lambda closure allocations
added: Test for dadhi#116
Leszek-Kowalski pushed a commit to Leszek-Kowalski/DryIoc that referenced this issue Oct 11, 2019
@Madajevas
Copy link

Madajevas commented Mar 16, 2023

Hi. It looks like this is still/again the case with DryIoc 5.3.2 (and 5.3.4) on .NET 4.8. My scenario:

using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;

using DryIoc;

using NUnit.Framework;

namespace YadaYada.Tests
{
    public class ContainerTests
    {
        interface/*abstract class*/ IQuery<T> { }
        class Query<T> : IQuery<T> { };
        class QueryDecorator<T> : IQuery<T>
        {
            public QueryDecorator(IQuery<T> decoratee) { }
        }

        [Test, Repeat(10)]
        public async Task Resolve_WhenCallInParallelToResolveScopedDecoratedService_FailsToResolveDecorator()
        {
            var container = new Container(rules => rules.WithDefaultReuse(Reuse.InWebRequest));

            container.Register(typeof(IQuery<string>), typeof(Query<string>));
            container.Register(typeof(IQuery<string>), typeof(QueryDecorator<string>), setup: Setup.Decorator);

            IQuery<string> ResolveInScope()
            {
                using (var scope = container.OpenScope(Reuse.WebRequestScopeName))
                {
                    return scope.Resolve<IQuery<string>>();
                }
            }

            IEnumerable<Task<IQuery<string>>> GetResolveTasks()
            {
                while (true)
                {
                    yield return Task.Run(() => ResolveInScope());
                }
            }

            var tasks = GetResolveTasks().Take(10).ToArray();
            await Task.WhenAll(tasks);

            foreach (var task in tasks)
            {
                Assert.That(task.Result, Is.InstanceOf<QueryDecorator<string>>());
            }
        }
    }
}

With VS "Running Until Failure" produces error right away.

Advice on workaround for time being would be welcome.

@dadhi
Copy link
Owner

dadhi commented Mar 16, 2023

@Madajevas Thanks for the test. Did it reproduce for you before 5.3.2?
Also could you check, what happens if you avoid using the named scope Reuse.InWebRequest and will use scope without name?

@Madajevas
Copy link

  • with rules.WithDefaultReuse(Reuse.InWebRequest) removed - fails
  • with var scope = container.OpenScope() - fails
  • resolving without scope, directly from container - fails

It seems to be working with 4.8.8 (completed 200+ iterations successfully. With v5.0.0 it takes up to 10 to fail).

This is still not conclusive. Will test further and post if results changes.

@dziedrius
Copy link

I was able to reproduce the issue on my machine, it would not always fail within 10 iterations, but with more iterations I can reproduce it constantly, seems to be some kind of tight race condition.

@dadhi dadhi reopened this Mar 16, 2023
dadhi added a commit that referenced this issue Mar 16, 2023
dadhi added a commit that referenced this issue Mar 16, 2023
@dadhi dadhi modified the milestones: v4.0.4, v5.4.0 Apr 28, 2023
dadhi added a commit that referenced this issue Apr 28, 2023
…prove;

optimize size of the Resgitry by splitting the Keyed services and Registry change to separate objects
dadhi added a commit that referenced this issue Apr 29, 2023
@dadhi dadhi closed this as completed in ae42382 Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants