Data race in static Kronos.Clock.annotatedNow.getter

[Usipov]

---

WARNING: ThreadSanitizer: data race (pid=97366)
  Read of size 8 at 0x0001164e9888 by thread T4:
    #0 static Clock.annotatedNow.getter <null>:2 (Kronos:x86_64+0x26a76ca)
    #1 static Clock.now.getter <null>:2 (Kronos:x86_64+0x26a747d)

  Previous write of size 8 at 0x0001164e9888 by main thread:
    #0 static Clock.stableTime.setter <null>:2 (Kronos:x86_64+0x26a6dcf)
    #1 closure #1 in static Clock.sync(from:samples:first:completion:) <null>:2 (Kronos:x86_64+0x26a827e)
    #2 partial apply for closure #1 in static Clock.sync(from:samples:first:completion:) <null>:2 (Kronos:x86_64+0x26a86b5)
    #3 $defer #1 () in closure #1 in closure #1 in NTPClient.query(pool:version:port:numberOfSamples:maximumServers:timeout:progress:) <null>:2 (Kronos:x86_64+0x26b7cf7)
    #4 closure #1 in closure #1 in NTPClient.query(pool:version:port:numberOfSamples:maximumServers:timeout:progress:) <null>:2 (Kronos:x86_64+0x26b783c)
    #5 partial apply for closure #1 in closure #1 in NTPClient.query(pool:version:port:numberOfSamples:maximumServers:timeout:progress:) <null>:2 (Kronos:x86_64+0x26bd6f8)
    #6 closure #1 in NTPClient.query(ip:port:version:timeout:numberOfSamples:completion:) <null>:2 (Kronos:x86_64+0x26b9af7)
    #7 partial apply for closure #1 in NTPClient.query(ip:port:version:timeout:numberOfSamples:completion:) <null>:2 (Kronos:x86_64+0x26bdb8e)
    #8 thunk for @escaping @callee_guaranteed (@guaranteed Data?, @unowned Double) -> () <null>:2 (Kronos:x86_64+0x26ba39d)
    #9 closure #1 in NTPClient.sendAsyncUDPQuery(to:port:timeout:completion:) <null>:2 (Kronos:x86_64+0x26bd0dd)
    #10 @objc closure #1 in NTPClient.sendAsyncUDPQuery(to:port:timeout:completion:) <null>:2 (Kronos:x86_64+0x26bd1f3)
    #11 __CFSocketPerformV0 <null>:2 (CoreFoundation:x86_64+0x92fd9)
    #12 start <null>:2 (libdyld.dylib:x86_64+0x1408)

  Location is global 'static Clock.stableTime' at 0x0001164e9888 (Kronos+0x000005b96888)

  Thread T4 (tid=1302869, running) is a GCD worker thread

[keith]

So technically right now we expect you to call .now and .sync from the same thread. In our app we only ever call this from the main thread. I think we'd be happy to support this if you wanted to submit a change for it though

[maxep]

Hey 👋

This issue has been reported on our side as well, see dd-sdk-ios/#588. Is it right to say that Clock.now should be invoked from the main thread? Given the current implementation, sockets are added to the CFRunLoopGetMain(), so all callback of NTPClient.query will be called on the main thread.

I think Kronos could benefit from 2 changes:

1. ~~Allow using the current run loop.~~ This won't work, see this thread. But we could still allow a queue to dispatch completion blocks. Something like this:

public static func sync(from pool: String = "time.apple.com", samples: Int = 4,
                        queue: DispatchQueue = .main,
                        first: ((Date, TimeInterval) -> Void)? = nil,
                        completion: ((Date?, TimeInterval?) -> Void)? = nil)
{
    self.loadFromDefaults()

    NTPClient().query(pool: pool, numberOfSamples: samples) { offset, done, total in
        queue.async {
            if let offset = offset {
                self.stableTime = TimeFreeze(offset: offset)

                if done == 1, let now = self.now {
                    first?(now, offset)
                }
            }

            if done == total {
                completion?(self.now, offset)
            }
        }
    }
}

2. Make TimeFreeze public so integration could create and use that object in a thread safe manner.

Would this make sense?