@pyrtsa Thanks for the PR and for looking into this! It's been a little while since we worked on this, but I believe our worry at the time was that two shared references could compete and initialize the same persistent reference at the same time, and that they would hold unique references while only a single one raced to live in the global persistent references store.

When looking into this did you conclude that this is not possible due to the existing locking?

I also wonder if it's possible to cook up a test case that deadlocks on main and is fixed in this branch?

I wrote it so that while two or more threads can indeed start setting up the shared reference, only one of them will succeed (while holding the lock another time), and all others will simply drop their half-initialised reference and grab the one already there.

I need to think a bit on the failing/fixing unit test case, maybe it's possible with some artificial delays. 🤔

There's a unit test now.

I tried to make the CI run the commit with just the failing test on top of main, but GHA got stuck somehow. (Edit: Maybe the GHA macOS runner breaks down with this test; see #157?)

Anyway, you can check out d188bfb and run tests manually to see that it indeed deadlocks.

@pyrtsa Can you explain why you added this? The tests seem to pass without it.

Hmm, I think I was being overly pedantic there, and it can be simplified down to:

      let reference = _PersistentReference(
        key: key,
        value: value,
        skipInitialLoad: skipInitialLoad
      )
      return lock.withLock {
        if let reference = (storage[key.id] as? Weak<Key>)?.reference {
          return reference
        } else {
          storage[key.id] = Weak(reference: reference)
          reference.onDeinit = { [self] in
            removeReference(forKey: key)
          }
          return reference
        }
      }

I originally added withExtendedLifetime in order to guarantee that the captured local _PersistentReference instance outlives the lock no matter what, because before this PR _PersistentReference used to access @Dependency(PersistentReferences.self) in its deinit, potentially causing the same deadlock (in even more subtle circumstances).

I wasn't sure if there was a possibility some other code path could access another @Dependency somewhere else. Reading code closer now, _PersistentReference still has quite a few members which has or could indirectly have a non-trivial deinit (_$perceptionRegistrar, key, subject, _loadError, _saveError, subscription at least), but neither seems currently likely to use @Dependency in their deinit.

But even if that was the case, it seems the current Swift compiler retains the captured-but-unused local variable reference longer than the capturing closure passed to lock.withLock { ... } even without explicitly extending its lifetime. So I guess we can indeed simplify it.

Having said that, I think it would be good to next check whether swift-dependencies could be updated not to hold the CachedValues.lock while running user-defined code. That would also remove many concerns and potential future regressions causing deadlocks.

Pushed out a fix, and also moved the happy path (just return reference) to the top with an if/else, as the guard/else seemed to make the logic harder to follow with such a long else branch.