add semantics for stopping tasks that have no start handler #77
Conversation
motivation: address shutdown for tasks which allocate resources on construction. before this change, only tasks that have been started are shutdown, this creates an issue when the resources are allocated on construction since the item may not get started at all (for example due to error in earlier task) and thus will never be shutdown and the resources will leak. changes: add sematics to request shutdown even if not started, and assume this sematics when registering with start: .none or with registerShutdown
tomerd
changed the title
| 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)) |
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think this is really good change, that will make live in many use cases much simpler. 👍🚀 Left some comments here and there for discussion.
Before we merge we should definitely make the distinction between use cases (and in what "modes" ServiceLifecycle can be used) more visible in documentation.
|
@fabianfett simplified based on discussion, ptal |
|
Admitting I did not have time to loop back here... Feel free to merge if you want to @tomerd |
motivation: address shutdown for tasks which allocate resources on construction. before this change, only tasks that have been started are shutdown, this creates an issue when the resources are allocated on construction since the item may not get started at all (for example due to error in earlier task) and thus will never be shutdown and the resources will leak.
changes: add semantics to request shutdown even if not started, and assume this se semantics when registering with
start: .noneor withregisterShutdown