Conversation
| } | ||
|
|
||
| return this._acquireSystemLockIfNeeded(async () => { | ||
| return await this._acquireSystemLockIfNeeded(async () => { |
| /** | ||
| * Return whether this invocation is still running. | ||
| */ | ||
| get running(): boolean { |
There was a problem hiding this comment.
What's our style guide say for boolean fields? Should this be isRunning?
There was a problem hiding this comment.
It doesn't say anything about that AFAICT (https://google.github.io/styleguide/tsguide.html).
Renamed to isRunning.
| id: 'started'; | ||
| child: ScriptChildProcess; | ||
| } | ||
| | {id: 'stopping'} |
There was a problem hiding this comment.
Does this state still have an associated child process?
There was a problem hiding this comment.
I don't think it's needed, but if it turns out to be then I will add it. I'm only adding state data as needed.
| type: 'info', | ||
| detail: 'service-started', | ||
| }); | ||
| this._state = { |
There was a problem hiding this comment.
Should the state transition after the promises below settle?
There was a problem hiding this comment.
No. The new state is started, and we set up a handler for the "all consumers done" event, which allows the service to shut down.
| this._config, | ||
| dependencyFingerprints.value | ||
| ); | ||
| this._state = {id: 'unstarted'}; |
There was a problem hiding this comment.
I think it's a little confusing to have the state set twice in this case. It's also a bit confusing to see some of these functions be async and use await and others use unawaited promises to set up the next transition... along with the boilerplate of the non-relevant states, I'd maybe in the future consider a state machine library, or some helpers to make the machine even more declarative.
There was a problem hiding this comment.
True. Replaced the async function with the event method style. This does require another state, but it makes sense. (The one other async function was just a mistake, it didn't actually await).
On the boilerplate: as this class grows, a lot more cases end up being meaningful. For now, I like that it forces me to consider every possible state. In some cases there will end up being only one valid state, so that could just be replaced with a simple assert at the top. Keeping it all consistent for now though.
2 participants
Just a start on the service state machine, with a test. Handling for the most basic simple case: one standard consumer that depends on one service. Obviously many cases not yet handled/tested here, but will build out more complexity incrementally.
Also fixes a bug with a missing
await, and adds a small test utility method.Part of #33