swift-service-lifecycle
swift-service-lifecycle copied to clipboard
swift-server
think about associated data when started
Manual state tracking has the advantage that you're able to associate data with a state. For example
enum LifecycleState {
case stopped
case started(Channel)
case stopping(Channel)
}
migrating this to Lifecycle currently would require to add an optional
var serverChannel: Channel?
and then the code would start to contain self.serverChannel!. The other option is to duplicate the state tracking in Lifecycle as well as the application but that's not nice either because the states can get out of sync.
I think this can be potentially achieved with the current API. check out the following test
func testExternalState() {
enum State: Equatable {
case idle
case started(String)
case shutdown
}
class Item {
let eventLoopGroup: EventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1)
let data: String
init(_ data: String) {
self.data = data
}
func start() -> EventLoopFuture<String> {
self.eventLoopGroup.next().makeSucceededFuture(self.data)
}
func shutdown() -> EventLoopFuture<Void> {
self.eventLoopGroup.next().makeSucceededFuture(())
}
}
var state = State.idle
let expectedData = UUID().uuidString
let item = Item(expectedData)
let lifecycle = Lifecycle()
lifecycle.register(name: "test",
start: .async{
item.start().map{ data -> Void in
state = .started(data)
}
},
shutdown: .async{
item.shutdown().map{ _ -> Void in
state = .shutdown
}
})
lifecycle.start(configuration: .init(shutdownSignal: nil)) { error in
XCTAssertNil(error, "not expecting error")
XCTAssert(state == .started(expectedData), "expected item to be shutdown, but \(state)")
lifecycle.shutdown()
}
lifecycle.wait()
XCTAssert(state == .shutdown, "expected item to be shutdown, but \(state)")
}
if you are trying to point out to a different issue, please elaborate
@tomerd sure, you can add manual state tracking again but then you also need to add locking etc. The test case as it stands today doesn't work because self.state gets read/written from different threads without synchronisation.
I was just hoping that we can come up with something better because otherwise we need to track the state in two places, think about locking, ...
Another thing to consider is what happens if we get a signal whilst holding a lock, then shutdown won't complete I think? It's just that lifecycle management is hard and I don't know if the library helps as much as it could? One option would be to give lifecycle some state? Maybe Lifecycle<StartedState, StoppedState>? Not sure if that's a good idea but i thought we should think about it.
Considering the synchronization concerns @weissi mentions it seems that Lifecycle should (optionally?) provide a way to safely assemble started/stopped states as it progresses through the up/down steps. Lifecycle<StartedState, StoppedState> sounds like an interesting idea, I imagine StartedState would be a non optional return of lifecycle.start?
@weissi @ddossot the original design thinking here was that state would be contained within each LifecycleItem. so, to @weissi's example if some unit of logic uses swift-nio and needs to track a Channel it would not be done externally to the Lifecycle but rather encapsulated in a LifecycleItem that gets started/stopped by Lifecycle.
If I understand your point correctly, the idea of Lifecycle<StartedState, StoppedState> could interesting if we feel different LifecycleItems need to co-operate towards some external, bigger, state that is composed from the state provided by different LifecycleItem start/stop return values. Is this what you have in mind?
Long story short: the var state = State.idle is terrifying (for concurrency reasons) and something I really really would want to not teach people that "it's fine". I outlined an API shape that could work below. If you'd like me to help out and PoC this I'm happy to.
So getting the type erasure dance will "the usual pain" but what we'd want is something among the lines of:
public class LifecycleItem<State> {
var label: String { get }
func start(callback: @escaping (Error?) -> State)
func shutdown(callback: @escaping (State?, Error?) -> Void)
}
// we'd need
let stages: [AnyLifecycleItem]
for stage in stages {
// ~~~ pseudo-code ~~~
let state = stage.invokeStart()
stage.store(state) // or just invokeStart stores it
}
for stage in shuttingDownStatges {
// ~~~ pseudo-code ~~~
stage.invokeShutdown() // shutdown(stage.storedState, maybeError)
}
we'd need type erasure dance around the storage of LifecycleItems
thanks @ktoso for the additional input.
Long story short: the var state = State.idle is terrifying (for concurrency reasons) and something I really really would want to not teach people that "it's fine"
of course, this is for illustration purposes
I outlined an API shape that could work below.
its sill not clear to me what is the advantage of the library offering an API around an item's state, unless items needs to expose their state to other items. iow, there is nothing in the current design preventing an item to manage it state in a contained way without the lifecycle library knowing / caring about it. what is the advantage of proposed API? do you see a need in items exposing their state to other items?
this is what I mean in https://github.com/swift-server/swift-service-launcher/issues/11#issuecomment-607976577
Based on previous experiences I think it's sufficient to keep any associated state/data within the individual LifecycleItems themselves--by this I am not suggesting we move the proposed feature from LifecycleState to LifecycleItem, but rather, if for example a HTTPServer "server" is a LifecycleItem and it has Channel, then Channel is an attribute of HTTPServer and not LifecycleState's.
Based on previous experiences I think it's sufficient to keep any associated state/data within the individual LifecycleItems themselves--by this I am not suggesting we move the proposed feature from LifecycleState to LifecycleItem, but rather, if for example a HTTPServer "server" is a LifecycleItem and it has Channel, then Channel is an attribute of HTTPServer and not LifecycleState's
yes. this was the original design and how I am using it too
As a new user to Lifecycle, I found the need to erase my EventLoopFuture<Channel> to EventLoopFuture<Void> a bit cumbersome/unintuitive. I had to store the result of start in a sideband promise so I could properly shutdown the Channel.
ex:
final class MetricsHTTPServer {
let lifecycle = ComponentLifecycle(label: "foo")
private(set) var channelPromise: EventLoopPromise<Channel>
let eventLoopGroup: EventLoopGroup
init(eventLoopGroup: EventLoopGroup) {
self.eventLoopGroup = eventLoopGroup
self.channelPromise = eventLoopGroup.next().makePromise(of: Channel.self)
lifecycle.register(
label: "Channel",
start: .eventLoopFuture(self.start),
shutdown: .eventLoopFuture(self.shutdown))
}
func start() -> EventLoopFuture<Void> {
let bootstrap = ServerBootstrap(group: eventLoopGroup)
// ...
let future = bootstrap.bind(host: ..., port: ...)
future.cascade(to: channelPromise)
return future.map { _ in }
}
func shutdown() -> EventLoopFuture<Void> {
return channelPromise.futureResult.flatMap { $0.close() }
}
}
As a user, I would've preferred to defer the storage of the Channel to the ComponentLifecycle and have it passed back to me as part of the shutdown callback.
I don't think this is solved. What I was looking for is a way to (from the outside) access what the start method constructed.
For example, what I seem to always need is something like this:
let service = ServiceLifecycle(configuration: .init(label: "proxy",
logger: logger,
callbackQueue: DispatchQueue.global(),
shutdownSignal: [.INT],
installBacktrace: true))
var downstreamClient: Client?
var otherComponent: Other?
var proxy: ProxyServer?
service.register(label: "downstream client",
start: .sync {
downstreamClient = try Client()
},
shutdown: .eventLoopFuture {
downstreamClient!.shutdown()
})
service.register(label: "other",
start: .sync {
other = try Other()
},
shutdown: .eventLoopFuture {
other!.shutdown()
})
service.register(label: "proxy",
start: .sync {
proxy = try Proxy(downstream: downstreamClient!, other: other!)
},
shutdown: .sync {
try proxy!.syncShutdown()
})
try service.startAndWait()
}
which seems very common but very ugly. The reason I want to use the start functionality is because if say the creation of Other fails, then DownstreamClient needs to be shut down (because it has been started before) but Proxy and Other don't (because they haven't been successfully started before).
@weissi did you notice the new registerStateful methods? they allow start to return a state which is then handed off to the corresponding shutdown. this solves at least some of the use case?
@tomerd yes, I did. But I need what's returned from start for the first service in the start of the third service, not (exclusively) in the shutdown. See where the !s are in my Proxy example up there, specifically:
// `downstreamClient` and `other` are created by the first 2 services.
proxy = try Proxy(downstream: downstreamClient!, other: other!)
@weissi I see. in most cases that need something like this, I saw at the "shared" instance created outside the lifecycle and the "starting" of it done in the start closure. e.g.
let downstreamClient = try Client()
let service = ServiceLifecycle(configuration: .init(label: "proxy",
logger: logger,
callbackQueue: DispatchQueue.global(),
shutdownSignal: [.INT],
installBacktrace: true))
service.register(label: "downstream client",
start: .sync {
downstreamClient.start()
},
shutdown: .eventLoopFuture {
downstreamClient.shutdown()
})
service.registerStateful(label: "proxy",
start: .sync {
return try Proxy(downstream: downstreamClient)
},
shutdown: .sync { proxy in
try proxy.syncShutdown()
})
try service.startAndWait()
That said, I can see how in some cases you would prefer to create the instance inside the start closure, especially for things that start when created and do not have a discrete start method. Given that start is (at its core) an async operation, the ergonomics of returning values from the start method to the calling context is a bit tricky, but with a/a I can see how we make it nice. wdyt?
@tomerd Right, if everything does have start methods, then this works. Unfortunately, most things don't actually come with a start method (I guess it's because it's one extra thing for the user to do). If they don't have start methods, they normally require their dependencies to be passed into init.
Regarding how we'd surface "return" values from start: Yes, there's definitely an API design question on how we can square sync, callbacks, futures, a/a and surface the value to the next stages.
Since we just changed the whole package to use a new Structured Concurrency approach, I would like to close this issue. @weissi Is that fine with you?
Since we just changed the whole package to use a new Structured Concurrency approach, I would like to close this issue. @weissi Is that fine with you?
Yes