Fix deadlock in CachedValues by using more narrow locking

[jtbergman]

Problem

In our project, we are hitting an interesting deadlock scenario with Swift Dependencies. It is quite convoluted, but it roughly works like this.

1. Object A acquires the CachedValues.lock and runs its liveValue initializer
2. Inside this initializer, we access additional dependencies while creating some subscriptions
3. The dependencies in the subscriptions are waiting for the lock and exhaust a thread pool
4. The initializer for Object A cannot complete until the subscriptions are ready, so the entire project deadlocks

This is an avoidable issue without any framework changes. We could:

1. Avoid using @Dependency inside the thread pool
2. Make the subscriptions asynchronously after initialization
3. Try to avoid having any slow liveValue initializers

Proposed Solution

However, we'd be a bit worried about this deadlock pattern (or other priority inversion) manifesting in other ways. The issue is fixed if we make the lock for CachedValues.value use a more narrow scoping as shown in this PR.

Let me know if you have any questions!

Additional Note on CachedValues.cache

Given that CachedValues.cache should be accessed via a lock I'm not sure it should be exposed publicly since the lock is private. The deprecated reset method that accesses it could be moved to CachedValues itself, and the test that sets _current.cachedValues.cached = [:] could use the reset method instead. This would require using @testable import Dependencies though and the usage seems deprecated anyway. Let me know if you'd like these changes made though.

[mbrandonw]

Hi @jtbergman, thanks for bringing this up and for investigating changes to the library! Stephen and I will discuss it soon, but one thing that would help a lot is if you could cook up a test case that deadlocks without your changes. Would it be possible to push that to this PR?

[jtbergman]

@mbrandonw Thanks for the quick response. I'll see what I can do and try to push one later today 🙏

[mbrandonw]

@jtbergman Thanks!

Also, I went ahead and ran tests on this PR and it does seem like the change has caused one failure:

swift-dependencies/Tests/DependenciesTests/DependencyValuesTests.swift

Line 654 in 0ef772d

func testThreadSafety() async {

This test fires up 10 tasks in a group to access an @Dependency and confirms that only one single dependency is init'd. With the changes in this PR that test failures because now code can interleave in the cache checks, causing two threads to see that no value exists in the cache and thus creates two dependencies. I believe that this test does capture the behavior we want from @Dependency and so we would want this test to pass, but we're open to discussing more about this.

I will say that the less you do in a dependency initializer, the better overall. Many people have run into issues trying to force a dependency to be created on the correct thread, or initialized in a specific manner, and it's always ended poorly. It's just the cost of having a very simple syntax like @Dependency(\.whatever) that can be used anywhere.

[jtbergman]

Hi @mbrandonw,

Thanks for the response! I initially misread the code, and thought that accessing the dependency before it's in the cache would cause a runtime warning (which I thought might be preferable to deadlock), but now I see that it causes the dependency value to be initialized multiple times.

I did, however, push some changes that show an example of the deadlock. If you run testNoDeadlock on aa1c32f you will see it fails and then passes on bf5d6d3. The example is super contrived, but the way it appeared in our codebase was more natural – essentially subscribing to a few Publishers with @Dependency inside them.

That being said, I agree that the failing test has the correct behavior, so the fix in bf5d6d3 should not land. One idea I had was that the thread should (1) create the dependency if it did not exist, (2) wait for the lock if the dependency is initializing, and (3) return the value if it is loaded. I thought a condition variable (via NSCondition) might be a good way to implement this.

I tried that approach and pushed it here. It fixes the currently broken test and actually works quite well, but it seems to cause issues in the tests named testAsyncStream.... This is unsurprising because I remember one of the Swift Concurrency WWDC talks called out:

It is unsafe to use thread-local storage, semaphores, conditions variables, and pthread_rw_lock with async/await. This is because these primitives have hidden dependencies that Swift Concurrency relies on for task scheduling.

In theory, this could probably be fixed by implementing some sort of spin lock mechanism to replace the condition variable. But given the issues that could introduce, plus the difficulty of even creating a deadlocking example for this issue, I think it's probably better to just leave the code in its current state.

Happy to close this, and we'll just fix in our codebase, but I appreciate the feedback 😄