Re-work thread safety mechanisms for flush process #611
Conversation
Sources/Flush.swift
Outdated
| if let requestData = requestData { | ||
| let semaphore = DispatchSemaphore(value: 0) | ||
| #if os(iOS) | ||
| flushBatchQueue.sync { |
There was a problem hiding this comment.
put all calls to flushQueueInBatches into sync queue in case it's being called from multiple threads
Sources/Flush.swift
Outdated
| #endif // os(iOS) | ||
| let success = flushRequest.sendRequest(requestData, | ||
| type: type, | ||
| useIP: useIPAddressForGeoLocation) |
There was a problem hiding this comment.
move the semaphore into sendRequest and make it synchronous from the perspective of flushQueueInBatches and return a boolean indicating success.
Sources/FlushRequest.swift
Outdated
| }) | ||
| _ = semaphore.wait(timeout: DispatchTime.distantFuture) |
There was a problem hiding this comment.
Maybe, we should change this timeout to some number of minutes?
Sources/Flush.swift
Outdated
| if let requestData = requestData { | ||
| let semaphore = DispatchSemaphore(value: 0) | ||
| #if os(iOS) | ||
| flushBatchQueue.sync { |
There was a problem hiding this comment.
calling .sync could potentially cause the deadlock.
jaredmixpanel
changed the title
|
Sorry to comment on an already-merged MR, but are you guys open to a Swift Concurrency/async-await refactor? I have a bunch of concerns with Flush.swift:
It seems like a lot of what you want to accomplish could be better handled in pure-swift with:
@zihejia @jaredmixpanel I can see you guys support back to iOS 11 (!!) in the Package.swift Would you consider raising the minimum OS to iOS 13.0, or even 14.0? Apple back-ported Swift Concurrency to 13.0, and that unlocks a whole world of feature improvements. If some PM gripes about legacy users, you could always fork this repo (as it is fairly stable) as mixpanel_legacy, and leave it up who insist on using Paleolithic-era OSes while the main repo forges ahead. |
Hopefully a fix for #578