From e62c10d0785e0fba7b28b91720590660b33d3ebd Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Wed, 9 Mar 2022 14:25:21 +1100 Subject: [PATCH 1/6] Use `self.` over ivar access for `networkMonitor` for consistency All other usages across the class are via `self.`. --- Sources/Event Logging/TracksService.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/Event Logging/TracksService.m b/Sources/Event Logging/TracksService.m index add38306..a48db50e 100644 --- a/Sources/Event Logging/TracksService.m +++ b/Sources/Event Logging/TracksService.m @@ -319,10 +319,10 @@ - (void)startNetworkMonitor { __weak typeof(self) weakSelf = self; self.networkMonitor = nw_path_monitor_create(); - nw_path_monitor_set_update_handler(_networkMonitor, ^(nw_path_t _Nonnull path) { + nw_path_monitor_set_update_handler(self.networkMonitor, ^(nw_path_t _Nonnull path) { [weakSelf networkPathChanged:path]; }); - nw_path_monitor_start(_networkMonitor); + nw_path_monitor_start(self.networkMonitor); } - (void)stopNetworkMonitor { From c3cd4bfb826164228d54b811ad801b59a5a2616c Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Wed, 9 Mar 2022 14:26:39 +1100 Subject: [PATCH 2/6] Move a `weakSelf` definition closer to its usage Source code locality helps local reasoning. --- Sources/Event Logging/TracksService.m | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Sources/Event Logging/TracksService.m b/Sources/Event Logging/TracksService.m index a48db50e..4c2d9547 100644 --- a/Sources/Event Logging/TracksService.m +++ b/Sources/Event Logging/TracksService.m @@ -317,8 +317,9 @@ - (void)startNetworkMonitor { return; } - __weak typeof(self) weakSelf = self; self.networkMonitor = nw_path_monitor_create(); + + __weak typeof(self) weakSelf = self; nw_path_monitor_set_update_handler(self.networkMonitor, ^(nw_path_t _Nonnull path) { [weakSelf networkPathChanged:path]; }); From 5e4240a22573a53d626f4157894b1aebd32ba25d Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Wed, 9 Mar 2022 14:27:36 +1100 Subject: [PATCH 3/6] =?UTF-8?q?Remove=20a=20double=20space=20=E2=80=94=20O?= =?UTF-8?q?ne=20is=20enough=20:)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/Event Logging/TracksService.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Event Logging/TracksService.m b/Sources/Event Logging/TracksService.m index 4c2d9547..3f1f00eb 100644 --- a/Sources/Event Logging/TracksService.m +++ b/Sources/Event Logging/TracksService.m @@ -320,7 +320,7 @@ - (void)startNetworkMonitor { self.networkMonitor = nw_path_monitor_create(); __weak typeof(self) weakSelf = self; - nw_path_monitor_set_update_handler(self.networkMonitor, ^(nw_path_t _Nonnull path) { + nw_path_monitor_set_update_handler(self.networkMonitor, ^(nw_path_t _Nonnull path) { [weakSelf networkPathChanged:path]; }); nw_path_monitor_start(self.networkMonitor); From 06c638bdc458a8a333c60f7172225efcc2c49200 Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Wed, 9 Mar 2022 14:34:27 +1100 Subject: [PATCH 4/6] Add a queue on which to execute the network monitor event handlers Without one, the update block would not run on start, resulting in 100% launches being marked as offline. Hat tip https://msolarana.netlify.app/2018/08/04/monitoring-network-changes/ --- Sources/Event Logging/TracksService.m | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Sources/Event Logging/TracksService.m b/Sources/Event Logging/TracksService.m index 3f1f00eb..191b5c5e 100644 --- a/Sources/Event Logging/TracksService.m +++ b/Sources/Event Logging/TracksService.m @@ -24,6 +24,9 @@ @interface TracksService () @property (nonatomic, strong) TracksContextManager *contextManager; @property (nonatomic, strong) TracksDeviceInformation *deviceInformation; @property (nonatomic, strong) nw_path_monitor_t networkMonitor; +/// The queue on which the Network framework will dispatch all the executions of the update and +/// cancel events handlers. +@property (nonatomic, strong) dispatch_queue_t networkMonitorQueue; @property (nonatomic, strong) nw_path_t networkPath; @property (nonatomic, readonly) NSString *userAgent; @@ -319,6 +322,17 @@ - (void)startNetworkMonitor { self.networkMonitor = nw_path_monitor_create(); + // Create and set a queue for where the monitor can execute it's event handlers. + // + // Without one, the update handler doesn't get called when the monitor starts. + dispatch_queue_attr_t attrs = dispatch_queue_attr_make_with_qos_class( + DISPATCH_QUEUE_SERIAL, + QOS_CLASS_UTILITY, + DISPATCH_QUEUE_PRIORITY_DEFAULT + ); + self.networkMonitorQueue = dispatch_queue_create("com.automattic.tracks.network.monitor", attrs); + nw_path_monitor_set_queue(self.networkMonitor, self.networkMonitorQueue); + __weak typeof(self) weakSelf = self; nw_path_monitor_set_update_handler(self.networkMonitor, ^(nw_path_t _Nonnull path) { [weakSelf networkPathChanged:path]; From 0867adf74c7c1ea97117f2f9d50af85a6f748b4a Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Thu, 17 Mar 2022 16:35:21 +1100 Subject: [PATCH 5/6] Explicitly set 0 as the relative priority for the network path monitor queue --- Sources/Event Logging/TracksService.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Sources/Event Logging/TracksService.m b/Sources/Event Logging/TracksService.m index 191b5c5e..0f241ae4 100644 --- a/Sources/Event Logging/TracksService.m +++ b/Sources/Event Logging/TracksService.m @@ -327,8 +327,8 @@ - (void)startNetworkMonitor { // Without one, the update handler doesn't get called when the monitor starts. dispatch_queue_attr_t attrs = dispatch_queue_attr_make_with_qos_class( DISPATCH_QUEUE_SERIAL, - QOS_CLASS_UTILITY, - DISPATCH_QUEUE_PRIORITY_DEFAULT + 0, + 0 // The relative priority within the QOS class. Can range from 0 to -15. ); self.networkMonitorQueue = dispatch_queue_create("com.automattic.tracks.network.monitor", attrs); nw_path_monitor_set_queue(self.networkMonitor, self.networkMonitorQueue); From d607bdc52e5bef7a4c9995c7967515b8036eb9eb Mon Sep 17 00:00:00 2001 From: Gio Lodi Date: Fri, 18 Mar 2022 16:55:37 +1100 Subject: [PATCH 6/6] Use `dispatch_queue_create_with_target` for network path monitor Co-authored-by: BJ Homer --- Sources/Event Logging/TracksService.m | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/Sources/Event Logging/TracksService.m b/Sources/Event Logging/TracksService.m index 0f241ae4..6362d780 100644 --- a/Sources/Event Logging/TracksService.m +++ b/Sources/Event Logging/TracksService.m @@ -322,15 +322,14 @@ - (void)startNetworkMonitor { self.networkMonitor = nw_path_monitor_create(); - // Create and set a queue for where the monitor can execute it's event handlers. + // Using `dispatch_queue_create_with_target` to create a serial queue that will target one of + // the existing concurrent views seems to be the approach Apple recommends. // - // Without one, the update handler doesn't get called when the monitor starts. - dispatch_queue_attr_t attrs = dispatch_queue_attr_make_with_qos_class( - DISPATCH_QUEUE_SERIAL, - 0, - 0 // The relative priority within the QOS class. Can range from 0 to -15. - ); - self.networkMonitorQueue = dispatch_queue_create("com.automattic.tracks.network.monitor", attrs); + // More at: https://github.com/Automattic/Automattic-Tracks-iOS/pull/199#discussion_r822683662 . + dispatch_queue_t utilityQueue = dispatch_get_global_queue(QOS_CLASS_UTILITY, 0); + self.networkMonitorQueue = dispatch_queue_create_with_target("com.automattic.tracks.network.monitor", + DISPATCH_QUEUE_SERIAL, + utilityQueue); nw_path_monitor_set_queue(self.networkMonitor, self.networkMonitorQueue); __weak typeof(self) weakSelf = self;