Services: Don't fail in watch mode when a dependency restarts or fails #526
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
augustjk
reviewed
Nov 14, 2022
src/test/service.test.ts
Outdated
|
|
||
| wireit.kill(); | ||
| await wireit.exit; | ||
| assert.not(service1Inv1.isRunning); |
There was a problem hiding this comment.
Noticed a failure here https://github.com/google/wireit/actions/runs/3465483821/jobs/5788267526
Maybe an extra 100ms wait before this line to de-flake?
AndrewJakubowicz
approved these changes
Nov 14, 2022
src/test/service.test.ts
Outdated
Comment on lines
1153
to
1154
| const service1 = await rig.newCommand(); | ||
| const service2 = await rig.newCommand(); |
There was a problem hiding this comment.
Unbelievably minor nit and can be discarded. Naming these parentService and childService could help make it easier to follow the test. I kept losing track of whether 1 or 2 was the child.
Also please ignore this comment.
There was a problem hiding this comment.
I can +1 that I did have to look at the config being written to tell which one was the dependent. But also, super minor since it is indeed right there.
AndrewJakubowicz
approved these changes
Nov 14, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In non-watch mode, if service A depends on service B, and service B exits unexpectedly, then service A is automatically killed. We do this because otherwise wireit will just continue running, even though there has been an error. We instead want to exit with a non-zero code.
Before this PR, we did the same thing in watch mode. However, this was wrong for 2 reasons:
If we're iteration N-1 which is about to be adopted into iteration N, our dependencies will sometimes intentionally restart, but we were treating this as though it was unexpected. This should not cause us to fail, since we'll either also restart very shortly (when cascade is true), or we'll just keep running (when cascade is false).
If we're iteration N and our dependency unexpectedly exits by itself, it's not actually helpful if we also aggressively exit. We can just keep running, and hopefully the user can fix the problem with the server dependency, which will then either cause us to restart or not depending on the cascade setting.
As part of this I found these other related issues, which were compounding the problem above and making it harder to debug:
We were returning
undefinedwhen we detached, if the service was in its "failing" or "stopping" state, instead of the child process. This was compounding the issue described above, because if iteration N-1 was failing, iteration N would think the child wasn't running at all, and would not wait for it to fully shut down. We are now much stricter about communicating from iteration N-1 to iteration N that there is a running child process, regardless of the state.We didn't log when a service shut down because of a failure in its dependencies.
Part of #33