Release/5.4.1 #810
Release/5.4.1 #810
Conversation
mscwilson
force-pushed
the
release/5.4.1
branch
2 times, most recently
from
72ae6e8
to
92f597d
Compare
There was a problem hiding this comment.
Nice to see everything in Swift!
I'm not much expert of the new Swift concurrency, especially for the test. But, it seems ok to me. Just the availability attribute is not right for macOS and watchOS.
About the issue with install_event. I think your solution works in practice but theoretically it leaves room to possible race conditions that can cause the same issue you are trying to fix. I can't say whether they would happen rarely or not but analysing the code I think they would be possible.
Probably it's ok as a temporary solution. I suggested an alternative solution but it's more complex to build.
| installTracker.clearPreviousInstallTimestamp() | ||
| if !installTracker.isNewInstall && previousTimestamp == nil { | ||
| return | ||
| DispatchQueue.global(qos: .default).async { [weak self] in |
There was a problem hiding this comment.
In the issue you say...
Currently, the application_install event is being generated before the additional callback configuration has been processed.
This works as expected in the Snowplow Android tracker.
I wonder why it works on Android. Your current solution puts this block in a queue for the execution. I think it works because it's queued after the fetcher that downloads and applies the remote configuration. I don't think this completely solves the problem. You can have race conditions where this block is executed before the fetching, ending up with the same issue we are trying to solve. Happy to be proved wrong btw.
In my opinion, this is a non trivial problem.
We should be sure that the install_event is tracked with the right configuration which is the one fetched by remote configuration. At the moment, there is no way for the Tracker class to know whether that Tracker instance comes from a default configuration or a remote one. Instead, this is an important information for install_event or other events. In this case we don't want to fire it until we have applied the remote configuration. So, in theory, we should make the Tracker class aware of our intention. For example: is it a tracker fully configured or we are waiting to fetch a remote config? If the Tracker is waiting for a remote config, it shouldn't fire any install_event until a timeout (defined somewhere) triggers the event.
That would exclude any possible race condition IMO.
There was a problem hiding this comment.
I agree, I'm not very happy with this solution, we should revisit this problem and plan it properly for a minor release in both iOS and Android. At least for now it will hopefully solve the majority of the problems for the bug reporter.
Tests/TestStateManager.swift
Outdated
| @@ -244,4 +244,22 @@ class TestStateManager: XCTestCase { | |||
| ) | |||
| ) | |||
| } | |||
|
|
|||
| @available(iOS 13, macOS 13, watchOS 13, tvOS 13, *) | |||
There was a problem hiding this comment.
Too many 13s here :)
For Task the requirements are written here https://developer.apple.com/documentation/swift/task
tvOS and iOS can be ok, but macOS and watchOS need to be lowered.
This is a patch release that fixes an occasional crash in the StateMachine manager, and ApplicationInstall events sent before updated tracker configuration.
Bug fixes
Add thread-safety to a globalContext manager function (#809)
Fix application_install event sent with wrong configuration (#808)