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

Add deadlock sample unit test #76

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

KaiOelfke
Copy link

I tried to make a unit test for the deadlock behavior that was discussed in the point free slack. I recently had a deadlock issue in a side project with PersistentContainer and the printing debug reducer.

I explained the issue here.

I think this test replicates the issue I was running into.

  1. A dependency A gets resolved on a non-main thread and acquires the CachedValues.lock
  2. Another dependency B gets resolved on the main thread and wants to acquire the CachedValues.lock
  3. The test, live, or previewValue default value initializer however needs synchronous main thread access

I think a solution might be to change the locking in CachedValues:

 func value<Key: TestDependencyKey>(
    for key: Key.Type,
    context: DependencyContext,
    file: StaticString = #file,
    function: StaticString = #function,
    line: UInt = #line
  ) -> Key.Value where Key.Value: Sendable {
    self.lock.lock()
    defer { self.lock.unlock() }
    // ...
    switch context {
          case .live:
            value = _liveValue(key) as? Key.Value
          case .preview:
            value = Key.previewValue
          case .test:
            value = Key.testValue
          }
    // ...

When executing the live|preview|testValue do we need hold the lock? I think it's expected that this is only run once and then cached. But this run once behavior could be perhaps implemented with a different locking mechanism compared to the self.cached mutations, which need to stay isolated of course. I'm not sure here and need to study the code more on this though.

Or all dependencies must be designed to not synchronously access the main thread, when the default value is executed. If this is a requirement it should be documented. This is how @tgrapperon fixed the PersistentContainer dependency.

I hope this is helpful.

@KaiOelfke
Copy link
Author

Another approach is to document a dependency as requiring the main thread so users either eagerly resolve and initialize the dependency on app launch or in a way that's guaranteed to be on the main thread.

Perhaps there could be also a way to annotate such dependencies with something like a MainThreadDependency type. I'm not sure, if this is safe in all cases, but it fixes the test and passes all other tests on my mac.

public protocol MainThreadDependency {}
struct NeedsMainThreadDep: TestDependencyKey, MainThreadDependency {}

func value<Key: TestDependencyKey>(
    for key: Key.Type,
    context: DependencyContext,
    file: StaticString = #file,
    function: StaticString = #function,
    line: UInt = #line
  ) -> Key.Value where Key.Value: Sendable {
    if key is MainThreadDependency.Type, !Thread.isMainThread {
      return DispatchQueue.mainSync {
        return self.value(for: key, context: context)
      }
    }
    self.lock.lock()
    defer { self.lock.unlock() }
    // ...
}

@mbrandonw
Copy link
Member

Hi @KaiOelfke, thanks so much for the repro! We need a bit more time to look at this in depth, but hope to get to it later this week or next.

@SebastianBoldt
Copy link

Any updates related to that Issue? I think I run into the same situation of my App randomly not responding because it gets stuck in exact that line of code.

@mbrandonw
Copy link
Member

Hi @SebastianBoldt, no update right now. I haven't looked into this for awhile, but if I remember correctly I don't think there is much we can do in the library other than document the behavior. It seems when this happens it is because dependencies are making assumptions about what thread it is being executed on, and that seems to be the true problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants