Improve concurrency model using a single internal dispatch queue (close #820)

[matus-tomlein]

This PR contains a refactoring of the internal classes in the tracker in order to improve concurrency (following recommendations in issue #820) and make it better handle concurrent accesses. This should help with several open bug reports: #774, #817, #816.

Generally, there are the following types of changes:

• using serial dispatch queues in order to serialize access to properties within each object.
• changing mutable to immutable properties to remove the need to protect the access.
• some refactoring along the way.

Tracker

track function

I added an option to run it synchronously (using a function parameter). I realized this was necessary because of how Session is implemented. If the function was run async, the foreground/background event could be tracked with already changed session instead of the session at the time the event was tracked. Btw, this is also the case on the Android tracker which has an async track method, but in that case we added this workaround in the tracker.

We didn't catch this in the tests because the async behaviour was mocked by the MockDispatchQueueWrapper. I removed this wrapper, it's probably better not to mock the async behaviour because then we are not testing what is really happening.

TrackerData

To make it easier to reason about the tracker state and pass it around, I added a new wrapper class TrackerData. This holds the private properties of the tracker. It doesn't protect the access to the properties, the idea is that this will only be called from the Tracker private functions or from TrackerPayloadBuilder (see below).

The Tracker class still publishes the same properties to external classes (mainly TrackerControllerImpl) as it did before. However, it runs all the accesses on a serial dispatch queue in a sync manner.

TrackerPayloadBuilder

This is an internal class that basically extracts the payload building functions from the Tracker class. The reason for this was to make it easier to test and reason about these functions separately from the Tracker. Everything in this class is executed on the serial queue so can access the tracker data directly.

Subject

Here we just wrap access to the subject properties in a serial dispatch queue.

Session

I have added two serial dispatch queues here:

• dispatchQueue used to protect access to all session data and session updates
• backgroundUpdateQueue to queue background and foreground updates

Emitter

I added a serial queue to protect access to the internal properties, but have also tried to make some properties immutable so that we don't have to add unnecessary protections. In particular, I made the namespace, networkConnection, and eventStore immutable. This required a change in how they are instantiated – I moved all of them to be defined in the Emitter constructor.

DefaultNetworkConnection

Added a serial queue here as well to protect access to the mutable properties.

Other changes

EventSink test helper

This is just a helper in the tests to make it easier to check tracked events using plugins – instead of always defining a custom plugin as we did before, we can use this helper and be consistent in our tests.

[hakonk]

While I have not had time to look thoroughly at all the changes, I have a couple of thoughts:

• Several places internal properties and methods are dispatched ad hoc via the sync function. This is a good improvement, but I believe it could have been simplified by making a distinction between the execution context for internal types and public ones, where the public interface explicitly dispatches the internal code on the internal queue, whether it be async or sync. Perhaps it would be beneficial to have an async public interface as many analytics events are fire and forget? dispatchPrecondition(.on(internalQueue)) can be used in internal types to ensure that code is indeed dispatched on the correct queue. If all internal code is dispatched on the same serial queue, there is no need for explicit synchronization. If there is a need for more than one quality of service, a queue hierarchy can be employed. I think it would be wise to gradually introduce more concurrency where needed instead of preemptively making the code highly concurrent as this introduces more complexity.

• Making properties immutable is a good idea. That said, when mutating the properties of an instance of a reference type that is declared as let one may still get data races. If all code in internal classes is dispatched on the internal queue, this would not be a problem.

[matus-tomlein]

Thank you for the suggestions @hakonk, that's super valuable!

I agree with starting with one internal queue and moving the dispatch to the public interface does make a lot of sense. I have tried to accomplish this in the following way:

• Wrapped all the public controllers using wrapper classes that dispatch the work on the internal queue before executing it. You can see the ..IQWrapper classes in this folder.
• Removed all synchronization from internal classes, everything is now executed on the single internal queue. There is only one exception – network requests are made on a separate background queue (in the Emitter class).
• Added checks for dispatchPrecondition in a few relevant places to make sure we are on the internal queue.

While I agree with your point about having a "fire and forget" async interface for the tracker, I think this can only be applied to some of the tracker APIs. Most importantly, the track() function definitely needs to be async, probably also the flush in Emitter. However, I think functions that update the internal state of the tracker should by synchronous to provide a consistent experience for the user. For instance, I made all the setters for properties synchronous, so that when a user calls a setter and getter in sequence, the getter always returns the updated value.

If you have any more feedback, please let us know, really appreciate it!

[mscwilson]

LGTM!
Huge effort. This should make the tracker much safer against concurrency problems. The EventSink test improvements are nice too.

[AvdLee]

@matus-tomlein is there a timeline for this to get released? We see numerous of crashes that are likely fixed with this PR. 🙏

[mscwilson]

Hi @AvdLee, sorry to hear about the crashes. We have a couple of other features to finish first. We're aiming to release v6 by the end of January.