[iOS] Fix data race in PersistentFileLogSpec.swift

[hakonk]

Fix data race in PersistentFileLogSpec.swift by synchronizing concurrent access to a bool value with NSLock.

Why

When running the tests with TSan enabled, I discovered a data race in PersistentFileLogSpec:

static func appendEntrySync(log: PersistentFileLog, entry: String) {
    var didAppend = false
    log.appendEntry(entry: entry) { _ in
      didAppend.value = true // <- mutated on a private dispatch queue
    }
    // expect polls on another thread every 500 ms.
    expect(didAppend).toEventually(beTrue(), timeout: .milliseconds(500))
}

A similar issue was found in AppLauncherWithDatabaseMock and fixed accordingly.

How

Employ locking mechanism for synchronizing access to the mentioned bool:

final class Synchronized<T> {
  private var _storage: T
  private var lock = NSLock()

  var value: T {
    get {
      return lockAround {
        _storage
      }
    }
    set {
      lockAround {
        _storage = newValue
      }
    }
  }

  init(_ storage: Bool) {
    self._storage = storage
  }

  private func lockAround<U>(_ closure: () -> U) -> U {
    lock.lock()
    defer { lock.unlock() }
    return closure()
  }

}

Substitute Swift.Bool with Synchronized<Bool>:

static func appendEntrySync(log: PersistentFileLog, entry: String) {
    let didAppend = Synchronized(false)
    log.appendEntry(entry: entry) { _ in
      didAppend.value = true
    }
    expect(didAppend.value).toEventually(beTrue(), timeout: .milliseconds(500))
  }

Test Plan

Piggy back on existing tests as they touch on the relevant code paths.

[wschurman]

Can you elaborate a bit more on what the data race is? I don't doubt that there is one but from my read of the code a data race is okay since there's only one thread writing the value and the other is read polling. If the poll needs one additional loop due to a inconsistent read I think the test still is valid?

Definitely okay with moving forward with this solution, just want to clarify a bit before reviewing the code.

[hakonk]

@wschurman Thanks for the feedback. I agree it is not a very severe case. I often use TSan to find potential issues, and I find it useful to eliminate any data races such that one can rather catch the most severe ones if they were to arise.

[hakonk]

Under the "Why" headline, I provided a code snippet with comments denoting the data race. We seem to agree on the nature of it. I can provide some screen shots from Xcode with TSan if that helps clarify the issue.

[wschurman]

Thanks for the fix! I also looked into using swift actors here but they aren't compatible with our current callback structure.

Don't worry about the test failures. Just one minor set of updates and then I will go ahead and merge this.

[wschurman]

Suggested change

	private var lock = NSLock()
	private let lock = NSLock()

[wschurman]

Suggested change

	private var lock = NSLock()
	private let lock = NSLock()

[hakonk]

Awesome! Thank you