Crash due to concurrent access to cache

[danielepantaleone]

Hi,

We are experiencing some crashes due to simultaneous access to Factory Scope.Cache:

We traced down the a piece of code in which we access a @WeakLazyInjected wrapped service inside the class deinit (if we comment it, we don't experience the crash):

class MyClass {

    @WeakLazyInjected(\MyContainer.myService)
    private (set) var myService

    deinit {
        myService?.doSomething() // <-- Thread 1: Simultaneous accesses to 0x600003720330, but modification requires exclusive access
    }
}

Digging a bit in Factory code I managed to see that every access is protected by a mutex, so I can't explain why this is happening.

[hmlongco]

It's not a concurrency problem, so the mutex won't help. Nor would additional locking as that would lead to a deadlock.

You're calling reset on a container, which is clearing the cache, which is freeing an object, which is calling deinit, which is referencing an uninitialized injector, which is then attempting to resolve it AND cache it into the cache that you're clearing.

Your best bet at the moment would be to make sure that you use myService prior to the deinit. In the meantime, it may be that reset needs to copy the array prior to clearing it, thus maintaining existing references.

Will need to think on it a bit.

[hmlongco]

Not sure the copy will work as the end result of what you're doing would leave a copy of the object in the cache after reset... which kind of defeats the purpose. May simply expose the initialize flag so you could do something like...

deinit {
    if $myService.initialized {
        myService?.doSomething()
    }
}

In other words, you can see if it's resolved before accessing it, which would cause it to resolve if it hasn't yet done so.

This is a problem with Lazy and WeakLazy...

[danielepantaleone]

Or maybe we can just refactor our code not to perform operations inside deinit (which I don't like very much). We actually moved that piece of code out of deinit at the moment, but it was worth reporting the issue. Maybe someone else will stumble on this very same problem.

[hmlongco]

Still may add the myService.initialized flags as doing any kind of cleanup of that nature will resolve the instance, even if the instance was never resolved previously.

Or maybe $myService.optional?.doSomething()

Hmmm. Needs a better name.

[danielepantaleone]

I like the second the most. Maybe something like:

$myService.resolvedOrNil?.doSomething()

Something that suggests that resolution is not going to be performed in this specific case. I'm not very good at naming things 😅

[hmlongco]

Sounds good to me. See develop branch.

[danielepantaleone]

I just tried and it works perfectly 👍🏻

[hmlongco]

2.2.0 will release shortly.