Skip to content

add semantics for stopping tasks that have no start handler #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 26, 2020
Merged

add semantics for stopping tasks that have no start handler #77

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5678bcb
add sematics for stopping tasks that have no start handler
tomerd Oct 9, 2020
97c031b
fixup
tomerd Oct 9, 2020
b63dc3e
better state managment
tomerd Oct 15, 2020
3cb52c4
simplification
tomerd Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 33 additions & 6 deletions Sources/Lifecycle/Lifecycle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,36 @@ import Logging
/// Represents an item that can be started and shut down
public protocol LifecycleTask {
var label: String { get }
var shutdownIfNotStarted: Bool { get }
func start(_ callback: @escaping (Error?) -> Void)
func shutdown(_ callback: @escaping (Error?) -> Void)
}

extension LifecycleTask {
public var shutdownIfNotStarted: Bool {
return false
}
}

// MARK: - LifecycleHandler

/// Supported startup and shutdown method styles
public struct LifecycleHandler {
private let body: (@escaping (Error?) -> Void) -> Void
internal let noop: Bool

private init(_ callback: @escaping (@escaping (Error?) -> Void) -> Void, noop: Bool) {
self.body = callback
self.noop = noop
}

/// Initialize a `LifecycleHandler` based on a completion handler.
///
/// - parameters:
/// - callback: the underlying completion handler
/// - noop: the underlying completion handler is a no-op
public init(_ callback: @escaping (@escaping (Error?) -> Void) -> Void) {
self.body = callback
self.init(callback, noop: false)
}

/// Asynchronous `LifecycleHandler` based on a completion handler.
Expand Down Expand Up @@ -69,9 +83,9 @@ public struct LifecycleHandler {

/// Noop `LifecycleHandler`.
public static var none: LifecycleHandler {
return LifecycleHandler { callback in
return LifecycleHandler({ callback in
callback(nil)
}
}, noop: true)
}

internal func run(_ callback: @escaping (Error?) -> Void) {
Expand Down Expand Up @@ -362,7 +376,12 @@ public class ComponentLifecycle: LifecycleTask {
case .shuttingDown:
self.stateLock.unlock()
// shutdown was called while starting, or start failed, shutdown what we can
let stoppable = started < tasks.count ? Array(tasks.prefix(started + 1)) : tasks
let stoppable: [LifecycleTask]
if started < tasks.count {
stoppable = tasks.enumerated().filter { $0.offset <= started || $0.element.shutdownIfNotStarted }.map { $0.element }
} else {
stoppable = tasks
}
self._shutdown(on: queue, tasks: stoppable) {
callback(error)
self.shutdownGroup.leave()
Expand Down Expand Up @@ -488,8 +507,8 @@ extension LifecycleTasksContainer {
/// - label: label of the item, useful for debugging.
/// - start: `Handler` to perform the startup.
/// - shutdown: `Handler` to perform the shutdown.
public func register(label: String, start: LifecycleHandler, shutdown: LifecycleHandler) {
self.register(_LifecycleTask(label: label, start: start, shutdown: shutdown))
public func register(label: String, shutdownIfNotStarted: Bool? = nil, start: LifecycleHandler, shutdown: LifecycleHandler) {
self.register(_LifecycleTask(label: label, shutdownIfNotStarted: shutdownIfNotStarted, start: start, shutdown: shutdown))
}

/// Adds a `LifecycleTask` to a `LifecycleTasks` collection.
Expand All @@ -504,9 +523,17 @@ extension LifecycleTasksContainer {

internal struct _LifecycleTask: LifecycleTask {
let label: String
let shutdownIfNotStarted: Bool
let start: LifecycleHandler
let shutdown: LifecycleHandler

init(label: String, shutdownIfNotStarted: Bool? = nil, start: LifecycleHandler, shutdown: LifecycleHandler) {
self.label = label
self.shutdownIfNotStarted = shutdownIfNotStarted ?? start.noop
self.start = start
self.shutdown = shutdown
}

func start(_ callback: @escaping (Error?) -> Void) {
self.start.run(callback)
}
Expand Down
3 changes: 3 additions & 0 deletions Tests/LifecycleTests/ComponentLifecycleTests+XCTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ extension ComponentLifecycleTests {
("testNIOFailure", testNIOFailure),
("testInternalState", testInternalState),
("testExternalState", testExternalState),
("testNOOPHandlers", testNOOPHandlers),
("testShutdownOnlyStarted", testShutdownOnlyStarted),
("testShutdownIfNotStartedWhenAsked", testShutdownIfNotStartedWhenAsked),
]
}
}
145 changes: 145 additions & 0 deletions Tests/LifecycleTests/ComponentLifecycleTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ final class ComponentLifecycleTests: XCTestCase {
let items = (1 ... Int.random(in: 10 ... 20)).map { index -> LifecycleTask in
let id = "item-\(index)"
return _LifecycleTask(label: id,
shutdownIfNotStarted: false,
start: .sync {
dispatchPrecondition(condition: .onQueue(testQueue))
startCalls.append(id)
Expand Down Expand Up @@ -839,4 +840,148 @@ final class ComponentLifecycleTests: XCTestCase {
lifecycle.wait()
XCTAssertEqual(state, .shutdown, "expected item to be shutdown, but \(state)")
}

func testNOOPHandlers() {
let none = LifecycleHandler.none
XCTAssertEqual(none.noop, true)

let sync = LifecycleHandler.sync {}
XCTAssertEqual(sync.noop, false)

let async = LifecycleHandler.async { _ in }
XCTAssertEqual(async.noop, false)

let custom = LifecycleHandler { _ in }
XCTAssertEqual(custom.noop, false)
}

func testShutdownOnlyStarted() {
class Item {
let label: String
let sempahore: DispatchSemaphore
let failStart: Bool
let exptectedState: State
var state = State.idle

deinit {
XCTAssertEqual(self.state, self.exptectedState, "\"\(self.label)\" should be \(self.exptectedState)")
self.sempahore.signal()
}

init(label: String, failStart: Bool, exptectedState: State, sempahore: DispatchSemaphore) {
self.label = label
self.failStart = failStart
self.exptectedState = exptectedState
self.sempahore = sempahore
}

func start() throws {
self.state = .started
if self.failStart {
self.state = .error
throw InitError()
}
}

func shutdown() throws {
self.state = .shutdown
}

enum State {
case idle
case started
case shutdown
case error
}

struct InitError: Error {}
}

let count = Int.random(in: 10 ..< 20)
let sempahore = DispatchSemaphore(value: count)
let lifecycle = ServiceLifecycle(configuration: .init(shutdownSignal: nil))

for index in 0 ..< count {
let item = Item(label: "\(index)", failStart: index == count / 2, exptectedState: index <= count / 2 ? .shutdown : .idle, sempahore: sempahore)
lifecycle.register(label: item.label, start: .sync(item.start), shutdown: .sync(item.shutdown))
}

lifecycle.start { error in
XCTAssertNotNil(error, "expecting error")
lifecycle.shutdown()
}
lifecycle.wait()

XCTAssertEqual(.success, sempahore.wait(timeout: .now() + 1))
}

func testShutdownIfNotStartedWhenAsked() {
class DestructionSensitive {
let label: String
let failStart: Bool
let sempahore: DispatchSemaphore
var state = State.idle

deinit {
XCTAssertEqual(self.state, .shutdown, "\"\(self.label)\" should be shutdown")
self.sempahore.signal()
}

init(label: String, failStart: Bool = false, sempahore: DispatchSemaphore) {
self.label = label
self.failStart = failStart
self.sempahore = sempahore
}

func start() throws {
self.state = .started
if self.failStart {
self.state = .error
throw InitError()
}
}

func shutdown() throws {
self.state = .shutdown
}

enum State {
case idle
case started
case shutdown
case error
}

struct InitError: Error {}
}

let sempahore = DispatchSemaphore(value: 6)
let lifecycle = ServiceLifecycle(configuration: .init(shutdownSignal: nil))

let item1 = DestructionSensitive(label: "1", sempahore: sempahore)
lifecycle.register(label: item1.label, start: .sync(item1.start), shutdown: .sync(item1.shutdown))

let item2 = DestructionSensitive(label: "2", sempahore: sempahore)
lifecycle.registerShutdown(label: item2.label, .sync(item2.shutdown))

let item3 = DestructionSensitive(label: "3", failStart: true, sempahore: sempahore)
lifecycle.register(label: item3.label, start: .sync(item3.start), shutdown: .sync(item3.shutdown))

let item4 = DestructionSensitive(label: "4", sempahore: sempahore)
lifecycle.registerShutdown(label: item4.label, .sync(item4.shutdown))

let item5 = DestructionSensitive(label: "5", sempahore: sempahore)
lifecycle.register(label: item5.label, start: .none, shutdown: .sync(item5.shutdown))

let item6 = DestructionSensitive(label: "6", sempahore: sempahore)
lifecycle.register(label: item6.label, shutdownIfNotStarted: true, start: .sync(item3.start), shutdown: .sync(item6.shutdown))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixed feelings here tbh.

I would love to rather revisit the design I proposed a while ago which allows the shutdown to get values from the start. This way, if there was no start called there is no start value.

I.e.

// start, semantically equivalent to:  
//     () -> SomeValue / EventLoopFuture<SomeValue>
// shutdown, semantically equivalent to: 
//    (TaskStartResult<SomeValue, Error>) -> Void / EventLoopFuture<Void>
lifecycle.register(
    label: item6.label, 
    start: .sync(DestructionSensitive()), 
    shutdown: .sync { $0.success?.shutdown() }
)

etc... Something worth exploring? Should I prototype it or would you have a look @tomerd ?
I remain very worried about those "allocate object outside" and then mutate start/stop the thing from lifecycle callbacks without a communication channel between then somehow... (esp if threading was involved which we might get to because: #21 )

Copy link
Contributor Author

@tomerd tomerd Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely @ktoso - you are more than welcome to explore and prototype and alternative better design. note that in this case the register function will also need to return the instance that is created in start since it is needed "outside" as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah I see could be tricky... don't treat this comment as blocker -- I'll try to poke around if I could make it happen


lifecycle.start { error in
XCTAssertNotNil(error, "expecting error")
lifecycle.shutdown()
}
lifecycle.wait()

XCTAssertEqual(.success, sempahore.wait(timeout: .now() + 1))
}
}