Swinject: Recursive `resolve()` deadlocks with `synchronize()`-d resolver

[serges147]

I see that internally the SynchronizedResolver class uses container's SpinLock (which is NSLock) to make locks. Can we change the NSLock to NSRecursiveLock here?

BTW, here's my use case (simplified of course - I do have concurrency):

public class A {
    private let b: B
    init(with r: Resolver) {
        b = r.resolve(B.self)    // this dead locks!
    }
}
private class B {}

let c = Container()
let threadSafeResolver = c.synchronize()

c.register(A.self, factory: { _ in return A(with: threadSafeResolver) })
c.register(B.self, factory: { _ in return B() })

threadSafeResolver.resolve(A.self)

[serges147]

IMO, current behaviour is if not a bug but serious limitation. Any suggestions how to at least work-around it?

[mpdifran]

Why can't you just pass B into the constructor?

i.e:

public class A {
    private let b: B
    init(with b: B) {
        self.b = b
    }
}
private class B {}

c.register(A.self) { r in 
    let b = r.resolve(B.self)!
    return A(with: b) 
}

[serges147]

Becase i don't want expose dependency to B. Also this is simle example - in reality I resove 3-5 different services as well in A's init. Definitelly would like to keep all of them as implementation detals.

[serges147]

Note, that your code will deadlock as well.

[yoichitgy]

Why don't you use a container like this without passing a resolver to A?

c.register(A.self, factory: { resolver in return A(with: resolver.resolve(B.self)!) })
c.register(B.self, factory: { _ in return B() })
let threadSafeResolver = c.synchronize()
threadSafeResolver.resolve(A.self)

[serges147]

Let me try your code - I don't see big difference with mine, but may be I'm wrong... Will be back in 15 min

[serges147]

Here we are. Check out attached sample project - deadlocks as I thought.
SwinjectLock.zip

[serges147]

Changing SpinLock.swift#12...

from: private let lock = NSLock()
to: private let lock = NSRecursiveLock()

...immediately fixes my test__deadlocks_on_second_resolve

[mpdifran]

@yoichitgy I believe your solution is the same as mine.

[serges147]

@mpdifran It looks like solution, but if you debug inside A's factory you will see that resolver there is NOT thread safe one - that is the main topic of this issue. Moving around place where resolving happens (either inside init or outside of it) change nothing.

[mpdifran]

Ah ok gotcha. So then the core issue is the resolver passed into the register's factory block is not the same as the original one?

i.e. despite using threadsafeResolver to resolve A, the resolver passed to the factory block is not threadsafeResolver?

[serges147]

Exactly! And when you try to workaround this (by capturing "external" threadSafeResolver) then you get deadlock on nested resolve.

[jakubvano]

Going through your test cases, Swinject is behaving as expected

1. in test__deadlocks_on_second_resolve it is expected to deadlock - as @yoichitgy mentioned in #269 it is a wrong usage of API.
   
   
   
   1. in test__it_does_not_deadlocks__but_its_not_threadsafe it might be unintuitive that resolver in factory closure is not threadSafeResolver, but it is expected given our current implementation of SynchronizedResolver. This is the proper use of API for achieving thread safe resolution.
   
   

If you are experiencing issues while using 2nd way of resolving dependencies, it would be helpful if you could provide some reproducible example so we can take a closer look at the problem.

BTW: we are aware that the thread safety model is currently not ideal (#139), and are planning to simplify it in the future - so if you could provide the example of unintuitive / faulty behaviour it would also help us to redesign this feature ;)

[serges147]

"...wrong usage.." - please give me rationale why it's wrong, I mean fundamentally! Please, I really need it because currently this is the ONLY workaround I have.

"...it might be unintuitive..." - I would say it almost hidden. It's luck that I found it. I might be wrong, but I guess most of developers will expect that if they call synchronize() then they are safe - but they are not. And, sorry, but it's not funny.

Yes, 2nd way cause unpredictable behavior - I will try to make reproducible test project.

[jakubvano]

By "wrong usage" I mean usage, which was not intended, has clear alternative (using resolver from factory closure), and as such should not (will not?) be optimised around.

You may have misunderstood me - using synchronize() should guarantee you thread safety (obviously there may be bugs). I was saying, that in current implementation, instance passed into the factory closure is not the same as instance returned from synchronize() method, and that is OK.

[serges147]

Please find it("won't resolve the same instance")
https://github.com/serges147/Swinject/commit/8d45c223ee820823d7b6724357b3e24e5d849613
The test either fails or even crashes - like a charm ;)

[serges147]

"_... instance passed into the factory closure is not the same as instance returned from synchronize() method..._" - yes I know this. But what is expected is that different resolver will be also thread safe - that's clearly not. If it is not a bug, then using resolver from factory closure is not so clear&safe as you say. This is the root cause why I started to capture external (definitely thread safe) resolver - not because I like to misuse the API.

[serges147]

Just last 5 cents to "wrong usage" topic (I won't say a word anymore).
In the "ThreadSafety" (https://github.com/Swinject/Swinject/blob/master/Documentation/ThreadSafety.md) doc there are samples with dispatch_async where into @escaping block you capture external thread safe view of container. I would like to state that it generally the same approach as mine: you "store" resolver for later (async) use in block, but I capture it in the factory block - potato patato.

[yoichitgy]

@serges147, thanks for checking the documentation😀 Please let us know If you see deadlock even when you don't capture the resolver.

If you want to capture the resolver, you can write your own utility in your project. It's just the usecase is not for most of people using Swinject, so we're not going to change the lock to NSRecursiveLock.

Btw, synchronize() method should have been renamed to synchronized() during Swift 3 migration.

[serges147]

@yoichitgy, I don't see deadlock if I use factory's resolver.

Have you tried my test (serges147/Swinject@8d45c22) ? Please do so, because it still might represent use cases of some people who deal with concurrency and need dynamically create new instances via resolver.resolve(... as in my test the Generator class does.

[yoichitgy]

Still it makes no sense to change the lock to a recursive lock. SynchronizedResolver is designed to be an entry point of resolve method not to use a recursive lock for simplicity and maintenancebility. The design controls the lock state.

We are not going to change the lock to a recursive lock in the current design, but a new approach for concurrency may be accepted in PR #139. Thanks😃👍

[serges147]

OK, will wait for your PR#139. For now I just made a patch in my project. Should I open another thread (IMHO bug) regarding the crash I experience when using your "official" way for threadsafety? I feel like you guys kinda ignore the crash fact but more concentrating on my "weird" usage of Swinject. Anyway, I guess you may close this thread - I have nothing to add about deadlocks and recursive lock.

[SiarheiBarysenka]

@yoichitgy Hey, I experienced the same issue with resolving dependencies on the same thread, and it seems solution with the recursive lock would be satisfactory. Would you mind to provide an option for the user to pass his own synchronization object on init of container rather than initializing SpinLock as a fallback inside of init? So we could declare, let's say, LockProtocol that has only one func sync<T>(action: () -> T) -> T declaration (the same as current SpinLock has), adopt this protocol to current SpinLock, and change the Container initialization in the following way:

public final class Container {
    ...
    internal let lock: LockProtocol // Used by SynchronizedResolver.

    internal init(parent: Container? = nil, debugHelper: DebugHelper, lockFallback: LockProtocol) {
        ...
        self.lock = parent.map { $0.lock } ?? lockFallback
    }

    public convenience init(parent: Container? = nil, lockFallback: LockProtocol = SpinLock()) {
        self.init(parent: parent, debugHelper: LoggingDebugHelper(), lockFallback: lockFallback)
    }

    public convenience init(parent: Container? = nil, registeringClosure: (Container) -> Void, lockFallback: LockProtocol = SpinLock()) {
        self.init(parent: parent, lockFallback: lockFallback)
        registeringClosure(self)
    }

Please let me know if this scenario may potentially work for you, so I can provide a PR. The one drawback is that SpinLock should be marked as public in this case.

[serges147]

@ SiarheiBarysenka , I personally like your solution with the LockProtocol. And I guess it's possible to hide internal SpinLock as well - the lockFallback could be optional with = nil as default, so that in case of .none it will fallback (internally) to original SpinLock. But I'm not sure that you solution won't contradict with future (unclear to me) vision for threadsafety (PR #139).

[skerkewitz]

Hello,

so quite some time passed since the issue was created but the problem still remains.

I understand the idea that you should not use resolve() in such a way, but you should consider that in a legacy code project it is pretty hard to introduce a library like Swinject in a clean way right from the start. To do that I need to refactor a lot of code.

I run into the same problem as OP and a change from NSLock() to NSRecursiveLock() solve my problem.

[dimazen]

Hello.
Any updates on that?

[ergunkocak]

Hello,

Hmm PR #139 is merged ? Has anyone still experience the problem?

[ergunkocak]

I tested it with version 2.4.0 And problem exists. The solution of @skerkewitz works nicely.
Would you mind applying to code base?

[ergunkocak]

Hello,

It has been 2 weeks. I know you are not support of any product :) and probably really busy :( Just to ping in case this issue is missed.

Best

[jakubvano]

Quick fix implemented by #350