Conversation
First of a series of PRs to implement services. This just documents the feature and adds the config field.
…mise (#461) 1. Added a `started` promise property to `ScriptChildProcess`. We'll be re-using this class for services, and we'll need to know when a child process has spawned. We didn't need that exposed before, because all that mattered was the exit code. 2. Added events and logging specific to services. 3. Refactored the config types so that classes can hold copies of just a subset of the fields of a config, instead of the full object. In watch mode, service child processes will get handed-off _across iterations_, and potentially _across different build graphs_, so I wanted to make sure we don't have a memory leak relating to keeping around references to old build graphs (configs hold references to their dependencies, so potentially that keeps the full build graph live). 4. Added a `ServiceScriptConfig` which is guaranteed to have `service:false`, and a stub `ServiceScriptExecution` which currently can produce a fingerprint, but doesn't yet actually start/stop the service command. More incremental work towards [services](#33).
Adds 3 properties to analysis that are useful for services: 1. `isDirectlyInvoked` tells us whether a service is either the root script, or there's a path from the root script to the service that only passes through no-command scripts (which counts the same way). This is useful because being directly invoked means you should only exit when wireit exits; instead of when all scripts that depend on you have finished. 2. `services` lists the services that must be started before a script can start. This isn't quite as simple as "my dependencies which are services", again because of needing to traverse through no-command scripts. 3. `serviceConsumers` is the reverse of `services`. It's useful for services to know which scripts could *potentially* depend on a script, because once we start a service, we don't want to shut it down until we know that every depending script has either finished, or didn't need to run at all. Knowing how many to expect from the start makes this simpler. Also adds a unit test that invokes the `Analyzer` directly. All of our other tests are integration, but in this case it's useful to be able to check the analysis result directly. Also noticed our uvu test patterns were effectively suffix matches, instead of exact matches (I noticed because `analysis\.test\.js$` was accidentally matching both `analysis.test.js` and `errors-analysis.test.js`). Part of #33
…471) A refactoring which allows scripts to directly access full `Execution` instances, instead of just fingerprints as before. This is useful for the lazy startup/shutdown feature of services, because other scripts need to not only get the service's fingerprint, but also to have a `start` method they can call once they eventually decide whether they need to run or not. Also: - Upgraded the self-signed TLS certificate we use in tests, because after upgrading my Linux distro and/or my Node version, these tests were failing locally due to the key being too weak. - Renamed `script` to `config` in a few places to remove ambiguity. - Use `_foo` style for `protected` methods. - Enabled `noImplicitOverride` tsc flag Part of #33
Adds a `servicesNotNeeded` promise, which gets resolved when a script either knows it will never run, or when it has finished running, and hence no longer needs the services it depends on to be running. This is how services will keep track of how many dependents still need them to be running -- once there are none left, they can shut down (unless they are directly invoked). This is done via a new `BaseExecutionWithCommand` base class, which is only relevant to scripts with commands (because no-command scripts don't directly consume services). Also: - Pass `abort` promise down to services. Directly-invoked services will need this to know when to shut down on SIGINT. - Removed a redundant `throw` I noticed. Part of #33
Integrates services into standard scripts (though note services don't yet actually _do_ anything, that's coming up next). Summary: 1. Before a standard script runs, all of its services must have started. If any service failed to start, we fail. 2. While a standard script is still running, if any of its services unexpectedly shuts down, then we're in an invalid state and fail too. Part of #33
Adds a state diagram as documentation for services. Part of #33
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
More incremental progress on services: - Show service stdout/stderr - Make services wait for their own services to start - Make standard scripts and services fail when a service exits unexpectedly (both while they are running and before) - Wait for services to shut down before ending an execution, including a refactor to make this simpler Also: - Gracefully close IPC socket in our test processes on SIGINT. Fixes occasional ECONNRESET errors. - Make our test stdout/stderr matcher only consume *up to the match*, instead of also consuming everything beyond. - Minor renaming Part of #33
- Directly invoked scripts now start up immediately, and shut down when wireit receives `SIGINT`. - All services now shut down whenever an error occurs anywhere in the graph, regardless of the `FAILURE_MODE` (`"continue" | "no-new" | "kill"`). - Services start up in bottom-up order, and stop in top-down order. We stop in top-down order so that if exiting gracefully requires interacting with another service you depend on, that will be reliable. - Updated the test rig so that we can get a notification of when a child process receives `SIGINT`, instead of always exiting. This lets us control how long it takes for a child to exit after it has been killed, so that we can better validate the order that services stop. Part of #33
Implements the basic logic for passing running services across watch mode iterations: 1. The top-level `execute` method now returns a map of the services that are still running at the end of the execution. 2. We pass that service map to the next iteration, and pass the previous version of each service into its new version. 3. As soon as a service knows its fingerprint, we check if it matches the previous version's fingerprint. If it does match, we "adopt" it instead of starting a new process. If it does not match, we shut it down, and then continue as normal (effectively restarting it). Still a few cases to handle here, which are in TODOs, but will do in followup PRs. Part of #33
) Handles the case where we're in watch mode, and a service was entirely deleted from the graph. Previously it would keep running, but now we notice it and shut it down before starting the next execution. The same goes for scripts that used to be "directly invoked" and are now no longer (since those might not need to run at all). Part of #33
Allows scripts that consume services to cache. This just requires slightly relaxing how we define when a service is "fully tracked". A standard script needs both its inputs and outputs known in order to be "fully tracked", but services never have output so we need to not require it in their case.
Previously, only a directly invoked service persisted across watch iterations. But actually, if one of those services itself depends on a service, then that service should persist too.
So this PR renames "isDirectlyInvoked" to "isPersistent", and expands its definition to include services of services. We call non-persistent services "ephemeral".
Also allows `start` to be called multiple times (an obvious case to handle, just hadn't hit it in the tests so far).
Example:
```
start
(no-command)
/ \
▼ ▼
serve:api serve:static
(persistent service) (persistent service)
| |
▼ ▼
serve:db build:assets
(persistent service) (standard)
|
▼
serve:playwright
(ephemeral service)
```
Part of #33
I've started testing services for real on webcomponents.org, and I hit a bug where watch mode could get stuck because of some missing promise resolution. We were resolving the promises that indicate when a service is done only when it succeeded or failed, but not when it was aborted/didn't need to run at all. Part of #33
Previously, if we were shutting down a service in watch mode because either its fingerprint changed, or it was removed from the script graph, then during the time the service was shutting down, we wouldn't display its stdout/stderr. That was because we "detached" it before killing it, but detaching also includes removing the stdout/stderr listeners from the previous execution. It makes more sense to instead go through the existing `abort` process, and only `detach` when we are actually adopting a child into a new execution. Part of #33
Fixes two memory leaks that affected watch mode:
1. An `abort` promise which always remained unresolved until wireit exited. This prevented both standard and service executions from ever being garbage collected, because they both awaited those promises indefinitely, even after those instances were stale.
The fix was to replace the promise entirely with explicit `abort()` methods, which turned out to be simpler and less error-prone anyway.
2. The `Executor` for watch mode iteration N was holding references to all persistent services from watch mode iteration N-1, and so on all the way back through all iterations, because of the `previousIterationServices` map that we pass forward across iterations.
The fix was to delete services from the map after we know they are adopted, breaking the reference chain.
This PR includes new garbage collection tests that use `FinalizationRegistry` to keep track of how many instances of `Executors` and `Executions` there are after a `global.gc()`. I'm just running that on a single OS/node version on CI, since I think it might be slightly flaky, plus I think that's good enough anyway.
In writing the GC tests, I also found a few other bugs with services -- some unhandled state transitions, and a timing issue where we weren't waiting for persistent services to be ready before indicating that the first phase of execution was done.
Part of #33
Kind of a grab bag of fixes mostly related to error handling. Part of #33
aomarks
marked this pull request as ready for review
November 5, 2022 17:57
aomarks
enabled auto-merge
November 5, 2022 18:04
justinfagnani
approved these changes
Nov 6, 2022
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Merges services into the main branch, since I think it's close enough that there's no issue with intermediate releases now, and I want to merge in #238
Part of #33 (which I won't close until it's released)