From 244bc008648fc9761db38f132eb28685228e2682 Mon Sep 17 00:00:00 2001 From: tom doron Date: Mon, 16 May 2022 18:49:33 -0700 Subject: [PATCH 1/2] shutdown() should cancel the signal handlers installed by start() motivation: allow easier testing of shutdown hooks changes: * introduce ServiceLifecycle.removeTrap which removes a trap * call ServiceLifecycle.removeTrap when setting up the shutdown hook * make the shutdown hook cleanup into a lifecycle task to ensure correct ordering * add tests * improve logging rdar://89552798 --- Sources/Lifecycle/Lifecycle.swift | 61 +++++++++++++++---- Sources/Lifecycle/Locks.swift | 4 +- .../ComponentLifecycleTests.swift | 4 +- .../ServiceLifecycleTests+XCTest.swift | 1 + .../ServiceLifecycleTests.swift | 43 +++++++++++++ 5 files changed, 96 insertions(+), 17 deletions(-) diff --git a/Sources/Lifecycle/Lifecycle.swift b/Sources/Lifecycle/Lifecycle.swift index 5d00351..7601a61 100644 --- a/Sources/Lifecycle/Lifecycle.swift +++ b/Sources/Lifecycle/Lifecycle.swift @@ -30,12 +30,22 @@ public protocol LifecycleTask { var shutdownIfNotStarted: Bool { get } func start(_ callback: @escaping (Error?) -> Void) func shutdown(_ callback: @escaping (Error?) -> Void) + var logStart: Bool { get } + var logShutdown: Bool { get } } extension LifecycleTask { public var shutdownIfNotStarted: Bool { return false } + + public var logStart: Bool { + return true + } + + public var logShutdown: Bool { + return true + } } // MARK: - LifecycleHandler @@ -317,9 +327,14 @@ public struct ServiceLifecycle { self.log("intercepted signal: \(signal)") self.shutdown() }, cancelAfterTrap: true) - self.underlying.shutdownGroup.notify(queue: .global()) { - signalSource.cancel() - } + // register cleanup as the last task + self.registerShutdown(label: "\(signal) shutdown hook cleanup", .sync { + // cancel if not already canceled by the trap + if !signalSource.isCancelled { + signalSource.cancel() + ServiceLifecycle.removeTrap(signal: signal) + } + }) } } @@ -343,22 +358,34 @@ extension ServiceLifecycle { public static func trap(signal sig: Signal, handler: @escaping (Signal) -> Void, on queue: DispatchQueue = .global(), cancelAfterTrap: Bool = false) -> DispatchSourceSignal { // on linux, we can call singal() once per process self.trappedLock.withLockVoid { - if !trapped.contains(sig.rawValue) { + if !self.trapped.contains(sig.rawValue) { signal(sig.rawValue, SIG_IGN) - trapped.insert(sig.rawValue) + self.trapped.insert(sig.rawValue) } } let signalSource = DispatchSource.makeSignalSource(signal: sig.rawValue, queue: queue) signalSource.setEventHandler { + // run handler first + handler(sig) + // then cancel trap if so requested if cancelAfterTrap { signalSource.cancel() + self.removeTrap(signal: sig) } - handler(sig) } signalSource.resume() return signalSource } + public static func removeTrap(signal sig: Signal) { + self.trappedLock.withLockVoid { + if self.trapped.contains(sig.rawValue) { + signal(sig.rawValue, SIG_DFL) + self.trapped.remove(sig.rawValue) + } + } + } + /// A system signal public struct Signal: Equatable, CustomStringConvertible { internal var rawValue: CInt @@ -433,7 +460,7 @@ struct ShutdownError: Error { public class ComponentLifecycle: LifecycleTask { public let label: String fileprivate let logger: Logger - internal let shutdownGroup = DispatchGroup() + fileprivate let shutdownGroup = DispatchGroup() private var state = State.idle([]) private let stateLock = Lock() @@ -596,13 +623,15 @@ public class ComponentLifecycle: LifecycleTask { private func startTask(on queue: DispatchQueue, tasks: [LifecycleTask], index: Int, callback: @escaping ([LifecycleTask], Error?) -> Void) { // async barrier - let start = { (callback) -> Void in queue.async { tasks[index].start(callback) } } - let callback = { (index, error) -> Void in queue.async { callback(index, error) } } + let start = { callback in queue.async { tasks[index].start(callback) } } + let callback = { index, error in queue.async { callback(index, error) } } if index >= tasks.count { return callback(tasks, nil) } - self.logger.info("starting tasks [\(tasks[index].label)]") + if tasks[index].logStart { + self.logger.info("starting tasks [\(tasks[index].label)]") + } let startTime = DispatchTime.now() start { error in Timer(label: "\(self.label).\(tasks[index].label).lifecycle.start").recordNanoseconds(DispatchTime.now().uptimeNanoseconds - startTime.uptimeNanoseconds) @@ -642,14 +671,16 @@ public class ComponentLifecycle: LifecycleTask { private func shutdownTask(on queue: DispatchQueue, tasks: [LifecycleTask], index: Int, errors: [String: Error]?, callback: @escaping ([String: Error]?) -> Void) { // async barrier - let shutdown = { (callback) -> Void in queue.async { tasks[index].shutdown(callback) } } - let callback = { (errors) -> Void in queue.async { callback(errors) } } + let shutdown = { callback in queue.async { tasks[index].shutdown(callback) } } + let callback = { errors in queue.async { callback(errors) } } if index >= tasks.count { return callback(errors) } - self.logger.info("stopping tasks [\(tasks[index].label)]") + if tasks[index].logShutdown { + self.logger.info("stopping tasks [\(tasks[index].label)]") + } let startTime = DispatchTime.now() shutdown { error in Timer(label: "\(self.label).\(tasks[index].label).lifecycle.shutdown").recordNanoseconds(DispatchTime.now().uptimeNanoseconds - startTime.uptimeNanoseconds) @@ -739,12 +770,16 @@ internal struct _LifecycleTask: LifecycleTask { let shutdownIfNotStarted: Bool let start: LifecycleHandler let shutdown: LifecycleHandler + let logStart: Bool + let logShutdown: Bool init(label: String, shutdownIfNotStarted: Bool? = nil, start: LifecycleHandler, shutdown: LifecycleHandler) { self.label = label self.shutdownIfNotStarted = shutdownIfNotStarted ?? start.noop self.start = start self.shutdown = shutdown + self.logStart = !start.noop + self.logShutdown = !shutdown.noop } func start(_ callback: @escaping (Error?) -> Void) { diff --git a/Sources/Lifecycle/Locks.swift b/Sources/Lifecycle/Locks.swift index 70f9fef..ace7823 100644 --- a/Sources/Lifecycle/Locks.swift +++ b/Sources/Lifecycle/Locks.swift @@ -81,7 +81,7 @@ extension Lock { /// - Parameter body: The block to execute while holding the lock. /// - Returns: The value returned by the block. @inlinable - internal func withLock(_ body: () throws -> T) rethrows -> T { + func withLock(_ body: () throws -> T) rethrows -> T { self.lock() defer { self.unlock() @@ -91,7 +91,7 @@ extension Lock { // specialise Void return (for performance) @inlinable - internal func withLockVoid(_ body: () throws -> Void) rethrows { + func withLockVoid(_ body: () throws -> Void) rethrows { try self.withLock(body) } } diff --git a/Tests/LifecycleTests/ComponentLifecycleTests.swift b/Tests/LifecycleTests/ComponentLifecycleTests.swift index 2f6df3c..8e09c92 100644 --- a/Tests/LifecycleTests/ComponentLifecycleTests.swift +++ b/Tests/LifecycleTests/ComponentLifecycleTests.swift @@ -53,7 +53,7 @@ final class ComponentLifecycleTests: XCTestCase { dispatchPrecondition(condition: .onQueue(.global())) XCTAssertTrue(startCalls.contains(id)) stopCalls.append(id) - }) + }) } lifecycle.register(items) @@ -92,7 +92,7 @@ final class ComponentLifecycleTests: XCTestCase { dispatchPrecondition(condition: .onQueue(testQueue)) XCTAssertTrue(startCalls.contains(id)) stopCalls.append(id) - }) + }) } lifecycle.register(items) diff --git a/Tests/LifecycleTests/ServiceLifecycleTests+XCTest.swift b/Tests/LifecycleTests/ServiceLifecycleTests+XCTest.swift index 3200ce1..69f7a60 100644 --- a/Tests/LifecycleTests/ServiceLifecycleTests+XCTest.swift +++ b/Tests/LifecycleTests/ServiceLifecycleTests+XCTest.swift @@ -35,6 +35,7 @@ extension ServiceLifecycleTests { ("testSignalDescription", testSignalDescription), ("testBacktracesInstalledOnce", testBacktracesInstalledOnce), ("testRepeatShutdown", testRepeatShutdown), + ("testShutdownCancelSignal", testShutdownCancelSignal), ] } } diff --git a/Tests/LifecycleTests/ServiceLifecycleTests.swift b/Tests/LifecycleTests/ServiceLifecycleTests.swift index cb950cf..f334984 100644 --- a/Tests/LifecycleTests/ServiceLifecycleTests.swift +++ b/Tests/LifecycleTests/ServiceLifecycleTests.swift @@ -271,4 +271,47 @@ final class ServiceLifecycleTests: XCTestCase { XCTAssertEqual(attempts, count) } + + func testShutdownCancelSignal() { + if ProcessInfo.processInfo.environment["SKIP_SIGNAL_TEST"].flatMap(Bool.init) ?? false { + print("skipping testRepeatShutdown") + return + } + + struct Service { + static let signal = ServiceLifecycle.Signal.ALRM + + let lifecycle: ServiceLifecycle + + init() { + self.lifecycle = ServiceLifecycle(configuration: .init(shutdownSignal: [Service.signal])) + self.lifecycle.register(GoodItem()) + } + } + + let service = Service() + service.lifecycle.start { error in + XCTAssertNil(error, "not expecting error") + kill(getpid(), Service.signal.rawValue) + } + service.lifecycle.wait() + + var count = 0 + let sync = DispatchGroup() + sync.enter() + let signalSource = ServiceLifecycle.trap(signal: Service.signal, handler: { _ in + count = count + 1 // not thread safe but fine for this purpose + sync.leave() + }, cancelAfterTrap: false) + + // since we are removing the hook added by lifecycle on shutdown, + // this will fail unless a new hook is set up as done above + kill(getpid(), Service.signal.rawValue) + + XCTAssertEqual(.success, sync.wait(timeout: .now() + 2)) + XCTAssertEqual(count, 1) + + signalSource.cancel() + ServiceLifecycle.removeTrap(signal: Service.signal) + } } From 08350fb740a4c7675dfd7ebaa3b6a1f47b5a00ae Mon Sep 17 00:00:00 2001 From: tomer doron Date: Mon, 16 May 2022 20:49:02 -0700 Subject: [PATCH 2/2] Update Tests/LifecycleTests/ServiceLifecycleTests.swift Co-authored-by: Yim Lee --- Tests/LifecycleTests/ServiceLifecycleTests.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/LifecycleTests/ServiceLifecycleTests.swift b/Tests/LifecycleTests/ServiceLifecycleTests.swift index f334984..36c4446 100644 --- a/Tests/LifecycleTests/ServiceLifecycleTests.swift +++ b/Tests/LifecycleTests/ServiceLifecycleTests.swift @@ -274,7 +274,7 @@ final class ServiceLifecycleTests: XCTestCase { func testShutdownCancelSignal() { if ProcessInfo.processInfo.environment["SKIP_SIGNAL_TEST"].flatMap(Bool.init) ?? false { - print("skipping testRepeatShutdown") + print("skipping testShutdownCancelSignal") return }