If all the charms in a space become disabled, stop the scheduler for that space. #1194
Conversation
There was a problem hiding this comment.
Great, simple fix! Some comments on unhandled rejections
In the scenario where a scheduler/worker crashes, disables all charms in a space, and is recreated, would ensureCharms get called to eventually dispose of that worker? We could possibly have some timing issues in space-manager, but let's see where this gets us
| addCancel(scheduler.watch(didCharms)); | ||
| } | ||
|
|
||
| const removedSpaces = new Set(this.charmSchedulers.keys()).difference(dids); |
There was a problem hiding this comment.
TIL Set.prototype.difference
There was a problem hiding this comment.
Yeah, these functions are convenient. Particularly here, where the natural thing would be to modify the map as you walk through it, which can mess up your iterator.
| // we are no longer monitoring this space | ||
| const scheduler = this.charmSchedulers.get(did); | ||
| this.charmSchedulers.delete(did); | ||
| scheduler?.stop(); |
There was a problem hiding this comment.
Like shutting down old workers in space-manager.ts, this returns a promise mapping to the shutdown process, we don't want to await here, but we do want to handle a rejection here (or else we crash) -- consider something like scheduler?.stop().catch(e => console.error(Error stopping scheduler: ${e}));
There was a problem hiding this comment.
good point. i wish there were a good way for me to await.
Edit: I also did add your suggested error handling.
When the bgcharms cell is updated, that sink method called earlier has registered us for callbacks, so when we disable a charm (writing to the bgcharms cell), we should invoke ensureCharms with the updated result. You're right that this is a bit sticky, since we could have just done the write of the disabledAt in the worker. We do give a deactivation timeout in the stop function. I think this should all work correctly, though, and after the third failure, the worker will be stopped and removed (assuming that was the last enabled charm in that space). |
No description provided.