Skip to content

Conversation

kwasniew
Copy link
Collaborator

@kwasniew kwasniew commented Jul 22, 2025

About the changes

Lock based implementation replacing #128

Rationale:

  • DispatchQueue despite being a higher level abstraction is more difficult to reason about (sync updates, async updates, with barrier/without barrier). It's also unfamiliar to most devs working across many different programming languages
  • Our current DispatchQueue implementation was prone to deadlocks (reported by GoodNotes) and it took me a few iterations to remove them. With locks I got a working implementation on my first attempt
  • DispatchQueue based solution has similar LOC count as lock based solution
  • No Global Queue Deadlock Risk
  • Locks reduced full test suite time by 20% percent and slowest thread safety soak test by 40%
  • There's a higher level Actor based concurrency model but it doesn't support old iOS and macOS version that we still support

NSLock Implementation: Each component now uses a dedicated NSLock for thread-safe access to shared state:

  private let lock = NSLock()

  public var context: Context {
      get {
          lock.lock()
          let value = self._context
          lock.unlock()
          return value
      }
      set {
          lock.lock()
          self._context = newValue
          lock.unlock()
      }
  }

Each class manages its own lock to prevent cross-component deadlocks:

  • UnleashClientBase: Protects context updates and timer management
  • Poller: Synchronizes timer lifecycle and configuration updates
  • Metrics: Guards bucket operations and metrics collection
  • DictionaryStorageProvider: Thread-safe storage access

This PR also replaces Timer usage with DispatchSourceTimer

Important files

Discussion points

@kwasniew kwasniew changed the title fix: thread safety locks fix: threading issues with locks Jul 22, 2025
@kwasniew kwasniew requested a review from sjaanus July 23, 2025 07:22
@sjaanus
Copy link

sjaanus commented Jul 23, 2025

Generally looks ok, but I am not sure if all those locks are needed. It just feels weird that code needs to be filled with these little lock unlocks.

@github-project-automation github-project-automation bot moved this from New to Approved PRs in Issues and PRs Jul 23, 2025
@kwasniew kwasniew merged commit 050a68f into main Jul 23, 2025
6 checks passed
@kwasniew kwasniew deleted the fix-thread-safety-locks branch July 23, 2025 10:16
@github-project-automation github-project-automation bot moved this from Approved PRs to Done in Issues and PRs Jul 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants