Skip to content

Commit 541cf1b

Browse files
weissiweissi
authored
correctness: shutdown while hanging start (#19)
motivation: fix correctness issue when shutting down while hanging start use case changes: * only shutdown started items when shutting down while starting * refactor shutdown to use the configured callback queue from start, instead of a separate one * add correctness tests
1 parent ffe82de commit 541cf1b

File tree

4 files changed

+135
-17
lines changed

4 files changed

+135
-17
lines changed

‎Package.swift

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ import PackageDescription
44

55
let package = Package(
66
name: "swift-service-launcher",
7+
platforms: [
8+
.macOS(.v10_12),
9+
],
710
products: [
811
.library(name: "ServiceLauncher", targets: ["ServiceLauncher"]),
912
.library(name: "ServiceLauncherNIOCompat", targets: ["ServiceLauncherNIOCompat"]),

‎Sources/ServiceLauncher/Lifecycle.swift

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public class Lifecycle {
7575

7676
/// Shuts down the `LifecycleItem` array provided in `start` or `startAndWait`.
7777
/// Shutdown is performed in reverse order of items provided.
78-
public func shutdown(on queue: DispatchQueue = DispatchQueue.global()) {
78+
public func shutdown() {
7979
self.stateLock.lock()
8080
switch self.state {
8181
case .idle:
@@ -87,7 +87,7 @@ public class Lifecycle {
8787
case .shuttingDown, .shutdown:
8888
self.stateLock.unlock()
8989
return
90-
case .started(let items):
90+
case .started(let queue, let items):
9191
self.state = .shuttingDown
9292
self.stateLock.unlock()
9393
self._shutdown(on: queue, items: items) {
@@ -116,7 +116,7 @@ public class Lifecycle {
116116
}
117117
self.state = .starting
118118
}
119-
self._start(on: configuration.callbackQueue, items: items, index: 0) { _, error in
119+
self._start(on: configuration.callbackQueue, items: items, index: 0) { started, error in
120120
self.stateLock.lock()
121121
if error != nil {
122122
self.state = .shuttingDown
@@ -125,18 +125,19 @@ public class Lifecycle {
125125
case .shuttingDown:
126126
self.stateLock.unlock()
127127
// shutdown was called while starting, or start failed, shutdown what we can
128-
self._shutdown(on: configuration.callbackQueue, items: items) {
128+
let stoppable = started < items.count ? Array(items.prefix(started + 1)) : items
129+
self._shutdown(on: configuration.callbackQueue, items: stoppable) {
129130
callback(error)
130131
self.shutdownGroup.leave()
131132
}
132133
case .starting:
133-
self.state = .started(items)
134+
self.state = .started(configuration.callbackQueue, items)
134135
self.stateLock.unlock()
135136
configuration.shutdownSignal?.forEach { signal in
136137
self.logger.info("setting up shutdown hook on \(signal)")
137138
let signalSource = Lifecycle.trap(signal: signal, handler: { signal in
138139
self.logger.info("intercepted signal: \(signal)")
139-
self.shutdown(on: configuration.callbackQueue)
140+
self.shutdown()
140141
})
141142
self.shutdownGroup.notify(queue: DispatchQueue.global()) {
142143
signalSource.cancel()
@@ -208,7 +209,7 @@ public class Lifecycle {
208209
private enum State {
209210
case idle
210211
case starting
211-
case started([LifecycleItem])
212+
case started(DispatchQueue, [LifecycleItem])
212213
case shuttingDown
213214
case shutdown
214215
}

‎Tests/ServiceLauncherTests/LifecycleTests+XCTest.swift

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ extension Tests {
2626
static var allTests: [(String, (Tests) -> () throws -> Void)] {
2727
return [
2828
("testStartThenShutdown", testStartThenShutdown),
29-
("testImmediateShutdown", testImmediateShutdown),
29+
("testDispatchQueues", testDispatchQueues),
30+
("testShutdownWhileStarting", testShutdownWhileStarting),
31+
("testShutdownDuringHangingStart", testShutdownDuringHangingStart),
3032
("testBadStartup", testBadStartup),
3133
("testBadShutdown", testBadShutdown),
3234
("testStartAndWait", testStartAndWait),

‎Tests/ServiceLauncherTests/LifecycleTests.swift

Lines changed: 121 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,128 @@ final class Tests: XCTestCase {
4545
items.forEach { XCTAssertEqual($0.state, .shutdown, "expected item to be shutdown, but \($0.state)") }
4646
}
4747

48-
func testImmediateShutdown() {
49-
let items = (0 ... Int.random(in: 10 ... 20)).map { _ in GoodItem() }
48+
func testDispatchQueues() {
49+
let lifecycle = Lifecycle()
50+
let testQueue = DispatchQueue(label: UUID().uuidString)
51+
52+
lifecycle.register(label: UUID().uuidString,
53+
start: {
54+
dispatchPrecondition(condition: DispatchPredicate.onQueue(testQueue))
55+
},
56+
shutdown: {
57+
dispatchPrecondition(condition: DispatchPredicate.onQueue(testQueue))
58+
})
59+
lifecycle.register(label: UUID().uuidString,
60+
start: {
61+
dispatchPrecondition(condition: DispatchPredicate.onQueue(testQueue))
62+
},
63+
shutdown: {
64+
dispatchPrecondition(condition: DispatchPredicate.onQueue(testQueue))
65+
})
66+
lifecycle.start(configuration: .init(callbackQueue: testQueue, shutdownSignal: nil)) { error in
67+
XCTAssertNil(error)
68+
lifecycle.shutdown()
69+
}
70+
lifecycle.wait()
71+
}
72+
73+
func testShutdownWhileStarting() {
74+
class Item: LifecycleItem {
75+
let startedCallback: () -> Void
76+
var state = State.idle
77+
78+
let label = UUID().uuidString
79+
80+
init(_ startedCallback: @escaping () -> Void) {
81+
self.startedCallback = startedCallback
82+
}
83+
84+
func start(callback: @escaping (Error?) -> Void) {
85+
DispatchQueue.global().asyncAfter(deadline: .now() + 0.05) {
86+
self.state = .started
87+
self.startedCallback()
88+
callback(nil)
89+
}
90+
}
91+
92+
func shutdown(callback: (Error?) -> Void) {
93+
self.state = .shutdown
94+
callback(nil)
95+
}
96+
97+
enum State {
98+
case idle
99+
case started
100+
case shutdown
101+
}
102+
}
103+
var started = 0
104+
let startSempahore = DispatchSemaphore(value: 0)
105+
let items = (0 ... Int.random(in: 10 ... 20)).map { _ in Item {
106+
started += 1
107+
startSempahore.signal()
108+
} }
50109
let lifecycle = Lifecycle()
51110
lifecycle.register(items)
52111
lifecycle.start(configuration: .init(shutdownSignal: nil)) { _ in }
112+
startSempahore.wait()
53113
lifecycle.shutdown()
54114
lifecycle.wait()
55-
items.forEach { XCTAssertEqual($0.state, .shutdown, "expected item to be shutdown, but \($0.state)") }
115+
XCTAssertGreaterThan(started, 0, "expected some start")
116+
XCTAssertLessThan(started, items.count, "exppcts partial start")
117+
items.prefix(started).forEach { XCTAssertEqual($0.state, .shutdown, "expected item to be shutdown, but \($0.state)") }
118+
items.suffix(started + 1).forEach { XCTAssertEqual($0.state, .idle, "expected item to be idle, but \($0.state)") }
119+
}
120+
121+
func testShutdownDuringHangingStart() {
122+
let lifecycle = Lifecycle()
123+
let testQueue = DispatchQueue(label: UUID().uuidString)
124+
let blockStartGroup = DispatchGroup()
125+
let waitForShutdownGroup = DispatchGroup()
126+
let shutdownCompleteGroup = DispatchGroup()
127+
blockStartGroup.enter()
128+
waitForShutdownGroup.enter()
129+
shutdownCompleteGroup.enter()
130+
var startCalls = [String]()
131+
var stopCalls = [String]()
132+
133+
do {
134+
let id = UUID().uuidString
135+
lifecycle.register(label: id,
136+
start: {
137+
dispatchPrecondition(condition: DispatchPredicate.onQueue(testQueue))
138+
startCalls.append(id)
139+
blockStartGroup.wait()
140+
},
141+
shutdown: {
142+
dispatchPrecondition(condition: DispatchPredicate.onQueue(testQueue))
143+
XCTAssertTrue(startCalls.contains(id))
144+
stopCalls.append(id)
145+
waitForShutdownGroup.leave()
146+
})
147+
}
148+
do {
149+
let id = UUID().uuidString
150+
lifecycle.register(label: id,
151+
start: {
152+
dispatchPrecondition(condition: DispatchPredicate.onQueue(testQueue))
153+
startCalls.append(id)
154+
},
155+
shutdown: {
156+
dispatchPrecondition(condition: DispatchPredicate.onQueue(testQueue))
157+
XCTAssertTrue(startCalls.contains(id))
158+
stopCalls.append(id)
159+
})
160+
}
161+
lifecycle.start(configuration: .init(callbackQueue: testQueue, shutdownSignal: nil)) { error in
162+
XCTAssertNil(error)
163+
}
164+
lifecycle.shutdown()
165+
blockStartGroup.leave()
166+
waitForShutdownGroup.wait()
167+
lifecycle.wait()
168+
XCTAssertEqual(startCalls.count, stopCalls.count)
169+
XCTAssertEqual(startCalls.count, 1)
56170
}
57171

58172
func testBadStartup() {
@@ -68,15 +182,16 @@ final class Tests: XCTestCase {
68182
}
69183
}
70184

71-
let items: [LifecycleItem] = [GoodItem(), BadItem(), GoodItem()]
185+
let items: [LifecycleItem] = [GoodItem(), GoodItem(), BadItem(), GoodItem()]
72186
let lifecycle = Lifecycle()
73187
lifecycle.register(items)
74188
lifecycle.start(configuration: .init(shutdownSignal: nil)) { error in
75189
XCTAssert(error is TestError, "expected error to match")
76190
}
77191
lifecycle.wait()
78-
let goodItems = items.compactMap { $0 as? GoodItem }
79-
goodItems.forEach { XCTAssertEqual($0.state, .shutdown, "expected item to be shutdown, but \($0.state)") }
192+
let badItemIndex = items.firstIndex { $0 as? BadItem != nil }!
193+
items.prefix(badItemIndex).compactMap { $0 as? GoodItem }.forEach { XCTAssertEqual($0.state, .shutdown, "expected item to be shutdown, but \($0.state)") }
194+
items.suffix(from: badItemIndex + 1).compactMap { $0 as? GoodItem }.forEach { XCTAssertEqual($0.state, .idle, "expected item to be idle, but \($0.state)") }
80195
}
81196

82197
func testBadShutdown() {
@@ -386,11 +501,9 @@ final class Tests: XCTestCase {
386501
let item = GoodItem()
387502
lifecycle.register(label: "test",
388503
start: .async { callback in
389-
print("start")
390504
item.start(callback: callback)
391505
},
392506
shutdown: .async { callback in
393-
print("shutdown")
394507
item.shutdown(callback: callback)
395508
})
396509

@@ -407,7 +520,6 @@ final class Tests: XCTestCase {
407520

408521
let item = GoodItem()
409522
lifecycle.registerShutdown(label: "test", .async { callback in
410-
print("shutdown")
411523
item.shutdown(callback: callback)
412524
})
413525

0 commit comments

Comments
 (0)