From a9eeb7b6ad6d8ebaf085a031d620b46d93d08606 Mon Sep 17 00:00:00 2001 From: Radu Dan Date: Fri, 8 Nov 2024 12:49:03 +0200 Subject: [PATCH] Close sessions on invalidate --- .../Extension+Servicing.swift | 42 +++++++++++++------ macos/OuisyncFileProvider/Extension.swift | 19 ++++++++- ouisync | 2 +- 3 files changed, 48 insertions(+), 15 deletions(-) diff --git a/macos/OuisyncFileProvider/Extension+Servicing.swift b/macos/OuisyncFileProvider/Extension+Servicing.swift index e5b120350..1581c8733 100644 --- a/macos/OuisyncFileProvider/Extension+Servicing.swift +++ b/macos/OuisyncFileProvider/Extension+Servicing.swift @@ -70,23 +70,17 @@ extension Extension { let proxyId = generateProxyId() - connection.interruptionHandler = { [weak self] in + connection.interruptionHandler = { [weak self] in guard let self else { return } NSLog("😡 Connection to Ouisync XPC service has been interrupted") - if let s = self { - synchronized(s) { - s.proxies.removeValue(forKey: proxyId) - return - } + synchronized(self) { + _ = self.proxies.removeValue(forKey: proxyId) } } - connection.invalidationHandler = { [weak self] in + connection.invalidationHandler = { [weak self] in guard let self else { return } NSLog("😡 Connection to Ouisync XPC service has been invalidated") - if let s = self { - synchronized(s) { - s.proxies.removeValue(forKey: proxyId) - return - } + synchronized(self) { + _ = self.proxies.removeValue(forKey: proxyId) } } @@ -109,12 +103,34 @@ extension Extension { return false } + // this is a bit awkward because to avoid a reference leak, we have to atomically add + // our invalidation closure iff the base extension has not been shut down yet, but we + // also don't want to lift the rest of this function into the synchronized block, + // thus resulting in two checks against the same value + let active = synchronized(ext) { + if ext.active { + ext.invalidators.append { + connection.invalidate() // this should notify peer; invalidationHandler cleans up proxies + await ouisyncClient.close() + } + } + return ext.active + } + guard active else { + // presumably the OS is smart enough to not keep using this service provider bound + // to an instance it has already invalidate()d but stranger things have happened + NSLog("👋 The File Provider extension has received a connection from the app but is in the process of shutting down") + return false + } + let proxy = AppToBackendProxy(connection, sendToApp, ouisyncClient) connection.exportedObject = proxy connection.exportedInterface = NSXPCInterface(with: FromAppToFileProviderProtocol.self) - proxies[proxyId] = proxy + synchronized(self) { + proxies[proxyId] = proxy + } connection.resume() diff --git a/macos/OuisyncFileProvider/Extension.swift b/macos/OuisyncFileProvider/Extension.swift index bddc8fefb..4fceb050d 100644 --- a/macos/OuisyncFileProvider/Extension.swift +++ b/macos/OuisyncFileProvider/Extension.swift @@ -151,8 +151,25 @@ class Extension: NSObject, NSFileProviderReplicatedExtension { } } + // WARN: only mutate these while holding a lock on self + var active = true // this is set to false when shutting down + var invalidators: [() async -> Void] = [] // list of things called on invalidate(); only append if active = true! func invalidate() { - // TODO: cleanup any resources + let active = synchronized(self) { + defer { self.active = false } + return self.active + } + guard active else { return } + Task { + NSLog("👋 The File Provider extension is shutting down") + async let _ = ouisyncSession.client.close() + await withTaskGroup(of: Void.self) { + for invalidator in invalidators { + $0.addTask(operation: invalidator) + } + invalidators.removeAll() // just in case they were holding strong refs + } + } } func item(for identifier: NSFileProviderItemIdentifier, request: NSFileProviderRequest, completionHandler: @escaping (NSFileProviderItem?, Error?) -> Void) -> Progress { diff --git a/ouisync b/ouisync index a8955283c..23bb2433d 160000 --- a/ouisync +++ b/ouisync @@ -1 +1 @@ -Subproject commit a8955283cf275e259c08932f7fd97434a2e60e89 +Subproject commit 23bb2433d5e4662d385aca3d52fdbff99c09fdd8