From 1147c5ce0b579ff2b3cfa5f61efd081ddfdc6fdd Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 14 Oct 2022 13:34:53 -0700 Subject: [PATCH 01/18] Services: Documentation and config (#460) First of a series of PRs to implement services. This just documents the feature and adds the config field. --- CHANGELOG.md | 7 ++- README.md | 41 +++++++++++++ package.json | 10 ++++ schema.json | 4 ++ src/analyzer.ts | 98 +++++++++++++++++++++++++++++--- src/config.ts | 5 ++ src/test/errors-analysis.test.ts | 96 +++++++++++++++++++++++++++++++ src/test/service.test.ts | 35 ++++++++++++ 8 files changed, 288 insertions(+), 8 deletions(-) create mode 100644 src/test/service.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a961a500..5f8856cd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - +## [Unreleased] + +- Added `"service": true` setting, which is well suited for long-running + processes like servers. A service is started either when it is invoked directly, + or when another script that depends on it is ready to run. A service is stopped + when all scripts that depend on it have finished, or when Wireit is exited. ## [0.7.2] - 2022-09-25 diff --git a/README.md b/README.md index 8d26e1bc3..5eb25a60e 100644 --- a/README.md +++ b/README.md @@ -32,6 +32,7 @@ - [GitHub Actions caching](#github-actions-caching) - [Cleaning output](#cleaning-output) - [Watch mode](#watch-mode) +- [Services](#services) - [Failures and errors](#failures-and-errors) - [Package locks](#package-locks) - [Recipes](#recipes) @@ -388,6 +389,46 @@ The benefit of Wireit's watch mode over built-in watch modes are: simultaneously, such as build steps being triggered before all preceding steps have finished. +## Services + +By default, Wireit assumes that your scripts will eventually exit by themselves. +This is well suited for build and test scripts, but not for long-running +processes like servers. To tell Wireit that a process is long-running and not +expected to exit by itself, set `"service": true`. + +```json +{ + "scripts": { + "serve": "wireit", + "build:server": "wireit", + "build:assets": "wireit" + }, + "wireit": { + "serve": { + "command": "node my-server.js", + "service": true, + "files": ["server-config.json"], + "dependencies": ["build:server", "build:assets"] + } + } +} +``` + +If a service is run _directly_ (e.g. `npm run serve`), then it will stay running +until the user kills Wireit (e.g. `Ctrl-C`). + +If a service is a _dependency_ of one or more other scripts, then it will start +up before any depending script runs, and will shut down after all depending +scripts finish. + +In watch mode, a service will be restarted whenever one of its input files or +dependencies change. + +Services cannot have `output` files, because there is no way for Wireit to know +when a service has finished writing its output. If you have a service that +produces output, you should define a non-service script that depends on it, and +which exits when the service's output is complete. + ## Failures and errors By default, when a script fails (meaning it returned with a non-zero exit code), diff --git a/package.json b/package.json index 92d40b415..ee3fca9a1 100644 --- a/package.json +++ b/package.json @@ -47,6 +47,7 @@ "test:json-schema": "wireit", "test:optimize-mkdirs": "wireit", "test:parallelism": "wireit", + "test:service": "wireit", "test:watch": "wireit" }, "wireit": { @@ -88,6 +89,7 @@ "test:json-schema", "test:optimize-mkdirs", "test:parallelism", + "test:service", "test:watch" ] }, @@ -251,6 +253,14 @@ "files": [], "output": [] }, + "test:service": { + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"service\\.test\\.js$\"", + "dependencies": [ + "build" + ], + "files": [], + "output": [] + }, "test:watch": { "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"watch\\.test\\.js$\"", "dependencies": [ diff --git a/schema.json b/schema.json index 6848f6e39..d0fa29ca7 100644 --- a/schema.json +++ b/schema.json @@ -43,6 +43,10 @@ "type": "string" }, "type": "array" + }, + "service": { + "markdownDescription": "If true, treat this script as a long-running process.\nServices are automatically brought up and down as they are depended upon by other scripts. If invoked directly, services continue running until Wireit is killed with Ctrl-C.\nFor more info, see: https://github.com/google/wireit#services", + "type": "boolean" } }, "type": "object" diff --git a/src/analyzer.ts b/src/analyzer.ts index 2f8cb0a66..ab779d35f 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -462,6 +462,13 @@ export class Analyzer { command ); const clean = this._processClean(placeholder, packageJson, syntaxInfo); + const service = this._processService( + placeholder, + packageJson, + syntaxInfo, + command, + output + ); this._processPackageLocks(placeholder, packageJson, syntaxInfo, files); // It's important to in-place update the placeholder object, instead of @@ -476,7 +483,8 @@ export class Analyzer { dependencies, files, output, - clean: clean ?? true, + clean, + service, scriptAstNode: scriptCommand, configAstNode: wireitConfig, declaringFile: packageJson.jsonFile, @@ -741,9 +749,10 @@ export class Analyzer { placeholder: UnvalidatedConfig, packageJson: PackageJson, syntaxInfo: ScriptSyntaxInfo - ): undefined | boolean | 'if-file-deleted' { + ): boolean | 'if-file-deleted' { + const defaultValue = true; if (syntaxInfo.wireitConfigNode == null) { - return; + return defaultValue; } const clean = findNodeAtLocation(syntaxInfo.wireitConfigNode, ['clean']) as | undefined @@ -767,11 +776,86 @@ export class Analyzer { }, }, }); - // We shouldn't execute if there's failures, but just in case, this is - // likely the safest option. - return false; + return defaultValue; + } + return clean?.value ?? defaultValue; + } + + private _processService( + placeholder: UnvalidatedConfig, + packageJson: PackageJson, + syntaxInfo: ScriptSyntaxInfo, + command: JsonAstNode | undefined, + output: ArrayNode | undefined + ): boolean { + const defaultValue = false; + if (syntaxInfo.wireitConfigNode == null) { + return defaultValue; + } + const node = findNodeAtLocation(syntaxInfo.wireitConfigNode, [ + 'service', + ]) as undefined | JsonAstNode; + if (node == null) { + return defaultValue; + } + if (node.value !== true && node.value !== false) { + placeholder.failures.push({ + type: 'failure', + reason: 'invalid-config-syntax', + script: placeholder, + diagnostic: { + severity: 'error', + message: `The "service" property must be either true or false.`, + location: { + file: packageJson.jsonFile, + range: {length: node.length, offset: node.offset}, + }, + }, + }); + return defaultValue; + } + + const value = node?.value ?? defaultValue; + + if (value === true && command == null) { + placeholder.failures.push({ + type: 'failure', + reason: 'invalid-config-syntax', + script: placeholder, + diagnostic: { + severity: 'error', + message: `A "service" script must have a "command".`, + location: { + file: packageJson.jsonFile, + range: { + length: node.length, + offset: node.offset, + }, + }, + }, + }); } - return clean?.value; + + if (value === true && output != null) { + placeholder.failures.push({ + type: 'failure', + reason: 'invalid-config-syntax', + script: placeholder, + diagnostic: { + severity: 'error', + message: `A "service" script cannot have an "output".`, + location: { + file: packageJson.jsonFile, + range: { + length: output.node.length, + offset: output.node.offset, + }, + }, + }, + }); + } + + return value; } private _processPackageLocks( diff --git a/src/config.ts b/src/config.ts index 8fe978e9f..f2eae08a7 100644 --- a/src/config.ts +++ b/src/config.ts @@ -99,6 +99,11 @@ interface BaseScriptConfig extends ScriptReference { */ clean: boolean | 'if-file-deleted'; + /** + * Whether the script should run in service mode. + */ + service: boolean; + /** * The command string in the scripts section. i.e.: * diff --git a/src/test/errors-analysis.test.ts b/src/test/errors-analysis.test.ts index 767032ab9..0a69a5552 100644 --- a/src/test/errors-analysis.test.ts +++ b/src/test/errors-analysis.test.ts @@ -1737,4 +1737,100 @@ test( }) ); +test( + 'service is not a boolean', + timeout(async ({rig}) => { + await rig.write({ + 'package.json': { + scripts: { + a: 'wireit', + }, + wireit: { + a: { + command: 'true', + service: 1, + }, + }, + }, + }); + const result = rig.exec('npm run a'); + const done = await result.exit; + assert.equal(done.code, 1); + checkScriptOutput( + done.stderr, + ` +❌ package.json:8:18 The "service" property must be either true or false. + "service": 1 + ~` + ); + }) +); + +test( + 'service does not have command', + timeout(async ({rig}) => { + await rig.write({ + 'package.json': { + scripts: { + a: 'wireit', + b: 'wireit', + }, + wireit: { + a: { + service: true, + dependencies: ['b'], + }, + b: { + command: 'true', + }, + }, + }, + }); + const result = rig.exec('npm run a'); + const done = await result.exit; + assert.equal(done.code, 1); + checkScriptOutput( + done.stderr, + ` +❌ package.json:8:18 A "service" script must have a "command". + "service": true, + ~~~~` + ); + }) +); + +test( + 'service cannot have output', + timeout(async ({rig}) => { + await rig.write({ + 'package.json': { + scripts: { + a: 'wireit', + }, + wireit: { + a: { + command: 'true', + service: true, + output: ['foo'], + }, + }, + }, + }); + const result = rig.exec('npm run a'); + const done = await result.exit; + assert.equal(done.code, 1); + checkScriptOutput( + done.stderr, + ` +❌ package.json:9:17 A "service" script cannot have an "output". + "output": [ + ~ + "foo" + ~~~~~~~~~~~~~ + ] + ~~~~~~~` + ); + }) +); + test.run(); diff --git a/src/test/service.test.ts b/src/test/service.test.ts new file mode 100644 index 000000000..fb42dd613 --- /dev/null +++ b/src/test/service.test.ts @@ -0,0 +1,35 @@ +/** + * @license + * Copyright 2022 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import {suite} from 'uvu'; +import {WireitTestRig} from './util/test-rig.js'; + +const test = suite<{rig: WireitTestRig}>(); + +test.before.each(async (ctx) => { + try { + ctx.rig = new WireitTestRig(); + await ctx.rig.setup(); + } catch (error) { + // Uvu has a bug where it silently ignores failures in before and after, + // see https://github.com/lukeed/uvu/issues/191. + console.error('uvu before error', error); + process.exit(1); + } +}); + +test.after.each(async (ctx) => { + try { + await ctx.rig.cleanup(); + } catch (error) { + // Uvu has a bug where it silently ignores failures in before and after, + // see https://github.com/lukeed/uvu/issues/191. + console.error('uvu after error', error); + process.exit(1); + } +}); + +test.run(); From 525b62512d9c2e8dd04a28f75420328f7864d43e Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Mon, 17 Oct 2022 10:32:34 -0700 Subject: [PATCH 02/18] Services: Config types, events, stub execution class, and started promise (#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](https://github.com/google/wireit/issues/33). --- src/analyzer.ts | 39 +++++-- src/config.ts | 42 +++++-- src/event.ts | 93 ++++++++++------ src/execution/service.ts | 40 +++++++ src/executor.ts | 4 + src/logging/default-logger.ts | 13 +++ src/script-child-process.ts | 203 ++++++++++++++++++---------------- 7 files changed, 287 insertions(+), 147 deletions(-) create mode 100644 src/execution/service.ts diff --git a/src/analyzer.ts b/src/analyzer.ts index ab779d35f..a531fc785 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -1082,19 +1082,44 @@ export class Analyzer { }; return {ok: false, error: this._markAsInvalid(config, failure)}; } - { - const validConfig: ScriptConfig = { + + let validConfig: ScriptConfig; + if (config.service) { + // We should already have created an invalid script at this point, so we + // should never get here. We throw here to convince TypeScript that this + // is guaranteed. + if (config.command === undefined) { + throw new Error( + 'Internal error: Supposedly valid service did not have command' + ); + } + validConfig = { ...config, + state: 'valid', extraArgs: undefined, + dependencies: config.dependencies as Array>, + // Unfortunately TypeScript doesn't narrow the ...config spread, so we + // have to assign explicitly. + command: config.command, + }; + } else { + validConfig = { + ...config, state: 'valid', + extraArgs: undefined, dependencies: config.dependencies as Array>, + // Unfortunately TypeScript doesn't narrow the ...config spread, so we + // have to assign explicitly. + service: config.service, }; - // We want to keep the original reference, but get type checking that - // the only difference between a ScriptConfig and a - // LocallyValidScriptConfig is that the state is 'valid' and the - // dependencies are also valid, which we confirmed above. - Object.assign(config, validConfig); } + + // We want to keep the original reference, but get type checking that + // the only difference between a ScriptConfig and a + // LocallyValidScriptConfig is that the state is 'valid' and the + // dependencies are also valid, which we confirmed above. + Object.assign(config, validConfig); + return {ok: true, value: config as unknown as ScriptConfig}; } diff --git a/src/config.ts b/src/config.ts index f2eae08a7..32e6031fa 100644 --- a/src/config.ts +++ b/src/config.ts @@ -29,12 +29,30 @@ export interface ScriptReference extends PackageReference { name: string; } +/** + * A script with a defined command. + */ +export interface ScriptReferenceWithCommand extends ScriptReference { + /** + * The shell command to execute. + */ + command: JsonAstNode; + + /** + * Extra arguments to pass to the command. + */ + extraArgs: string[] | undefined; +} + export interface Dependency { config: Config; astNode: JsonAstNode; } -export type ScriptConfig = NoCommandScriptConfig | StandardScriptConfig; +export type ScriptConfig = + | NoCommandScriptConfig + | StandardScriptConfig + | ServiceScriptConfig; /** * A script that doesn't run or produce anything. A pass-through for @@ -43,21 +61,25 @@ export type ScriptConfig = NoCommandScriptConfig | StandardScriptConfig; export interface NoCommandScriptConfig extends BaseScriptConfig { command: undefined; extraArgs: undefined; + service: false; } /** * A script with a command that exits by itself. */ -export interface StandardScriptConfig extends BaseScriptConfig { - /** - * The shell command to execute. - */ - command: JsonAstNode; +export interface StandardScriptConfig + extends BaseScriptConfig, + ScriptReferenceWithCommand { + service: false; +} - /** - * Extra arguments to pass to the command. - */ - extraArgs: string[] | undefined; +/** + * A service script. + */ +export interface ServiceScriptConfig + extends BaseScriptConfig, + ScriptReferenceWithCommand { + service: true; } /** diff --git a/src/event.ts b/src/event.ts index d030c8f80..60963fbbc 100644 --- a/src/event.ts +++ b/src/event.ts @@ -9,6 +9,7 @@ import {Diagnostic} from './error.js'; import type { ScriptConfig, ScriptReference, + ScriptReferenceWithCommand, PackageReference, } from './config.js'; @@ -20,7 +21,7 @@ import type { */ export type Event = Success | Failure | Output | Info; -interface EventBase { +interface EventBase { script: T; diagnostic?: Diagnostic; diagnostics?: Diagnostic[]; @@ -32,35 +33,36 @@ interface EventBase { type Success = ExitZero | NoCommand | Fresh | Cached; -interface SuccessBase extends EventBase { +interface SuccessBase + extends EventBase { type: 'success'; } /** * A script finished with exit code 0. */ -export interface ExitZero extends SuccessBase { +export interface ExitZero extends SuccessBase { reason: 'exit-zero'; } /** * A script completed because it had no command and its dependencies completed. */ -export interface NoCommand extends SuccessBase { +export interface NoCommand extends SuccessBase { reason: 'no-command'; } /** * A script was already fresh so it didn't need to execute. */ -export interface Fresh extends SuccessBase { +export interface Fresh extends SuccessBase { reason: 'fresh'; } /** * Script output was restored from cache. */ -export interface Cached extends SuccessBase { +export interface Cached extends SuccessBase { reason: 'cached'; } @@ -90,16 +92,18 @@ export type Failure = | UnknownErrorThrown | DependencyOnMissingPackageJson | DependencyOnMissingScript - | DependencyInvalid; + | DependencyInvalid + | ServiceExitedUnexpectedly; -interface ErrorBase extends EventBase { +interface ErrorBase + extends EventBase { type: 'failure'; } /** * A script finished with an exit status that was not 0. */ -export interface ExitNonZero extends ErrorBase { +export interface ExitNonZero extends ErrorBase { reason: 'exit-non-zero'; status: number; } @@ -107,7 +111,7 @@ export interface ExitNonZero extends ErrorBase { /** * A script exited because of a signal it received. */ -export interface ExitSignal extends ErrorBase { +export interface ExitSignal extends ErrorBase { reason: 'signal'; signal: NodeJS.Signals; } @@ -115,7 +119,7 @@ export interface ExitSignal extends ErrorBase { /** * An error occured trying to spawn a script's command. */ -export interface SpawnError extends ErrorBase { +export interface SpawnError extends ErrorBase { reason: 'spawn-error'; message: string; } @@ -124,14 +128,14 @@ export interface SpawnError extends ErrorBase { * We decided not to start a script after all, due to e.g. another script * failure. */ -export interface StartCancelled extends ErrorBase { +export interface StartCancelled extends ErrorBase { reason: 'start-cancelled'; } /** * A script was intentionally and successfully killed by Wireit. */ -export interface Killed extends ErrorBase { +export interface Killed extends ErrorBase { reason: 'killed'; } @@ -162,15 +166,14 @@ export interface PackageJsonParseError extends ErrorBase { /** * The package.json doesn't have a "scripts" object at all. */ -export interface NoScriptsSectionInPackageJson - extends ErrorBase { +export interface NoScriptsSectionInPackageJson extends ErrorBase { reason: 'no-scripts-in-package-json'; } /** * The specified script does not exist in a package.json. */ -export interface ScriptNotFound extends ErrorBase { +export interface ScriptNotFound extends ErrorBase { reason: 'script-not-found'; diagnostic: Diagnostic; } @@ -179,8 +182,7 @@ export interface ScriptNotFound extends ErrorBase { * The specified script has a wireit config, but it isn't declared in the * scripts section at all. */ -export interface WireitScriptNotInScriptsSection - extends ErrorBase { +export interface WireitScriptNotInScriptsSection extends ErrorBase { reason: 'wireit-config-but-no-script'; diagnostic: Diagnostic; } @@ -188,7 +190,7 @@ export interface WireitScriptNotInScriptsSection /** * The specified script's command is not "wireit". */ -export interface ScriptNotWireit extends ErrorBase { +export interface ScriptNotWireit extends ErrorBase { reason: 'script-not-wireit'; diagnostic: Diagnostic; } @@ -201,7 +203,7 @@ export interface InvalidConfigSyntax extends ErrorBase { diagnostic: Diagnostic; } -export interface InvalidUsage extends ErrorBase { +export interface InvalidUsage extends ErrorBase { reason: 'invalid-usage'; message: string; } @@ -209,7 +211,7 @@ export interface InvalidUsage extends ErrorBase { /** * A script lists the same dependency multiple times. */ -export interface DuplicateDependency extends ErrorBase { +export interface DuplicateDependency extends ErrorBase { reason: 'duplicate-dependency'; /** * The dependency that is duplicated. @@ -221,8 +223,7 @@ export interface DuplicateDependency extends ErrorBase { /** * A script depends on another in a package that isn't there. */ -export interface DependencyOnMissingPackageJson - extends ErrorBase { +export interface DependencyOnMissingPackageJson extends ErrorBase { reason: 'dependency-on-missing-package-json'; diagnostic: Diagnostic; /** @@ -235,12 +236,19 @@ export interface DependencyOnMissingPackageJson /** * A script's dependency doesn't exist. */ -export interface DependencyOnMissingScript extends ErrorBase { +export interface DependencyOnMissingScript extends ErrorBase { reason: 'dependency-on-missing-script'; diagnostic: Diagnostic; supercedes: ScriptNotFound; } +/** + * A service exited before it was supposed to. + */ +export interface ServiceExitedUnexpectedly extends ErrorBase { + reason: 'service-exited-unexpectedly'; +} + /** * We reached the point of doing cyclic dependency checking, and one of our * transitive dependencies had not transitioned to being locally validated. @@ -251,7 +259,7 @@ export interface DependencyOnMissingScript extends ErrorBase { * continue to cycle detection even when some diagnostics were generated during * local analysis. */ -export interface DependencyInvalid extends ErrorBase { +export interface DependencyInvalid extends ErrorBase { reason: 'dependency-invalid'; dependency: UnvalidatedConfig; } @@ -259,7 +267,7 @@ export interface DependencyInvalid extends ErrorBase { /** * The dependency graph has a cycle in it. */ -export interface Cycle extends ErrorBase { +export interface Cycle extends ErrorBase { reason: 'cycle'; diagnostic: Diagnostic; @@ -268,7 +276,7 @@ export interface Cycle extends ErrorBase { /** * For when we catch an error not handled by any of the other types. */ -export interface UnknownErrorThrown extends ErrorBase { +export interface UnknownErrorThrown extends ErrorBase { reason: 'unknown-error-thrown'; error: unknown; } @@ -308,16 +316,19 @@ type Info = | OutputModified | WatchRunStart | WatchRunEnd + | ServiceStarted + | ServiceStopped | GenericInfo; -interface InfoBase extends EventBase { +interface InfoBase + extends EventBase { type: 'info'; } /** * A script's command started running. */ -export interface ScriptRunning extends InfoBase { +export interface ScriptRunning extends InfoBase { detail: 'running'; } @@ -325,7 +336,7 @@ export interface ScriptRunning extends InfoBase { * A script can't run right now because a system-wide lock is being held by * another process. */ -export interface ScriptLocked extends InfoBase { +export interface ScriptLocked extends InfoBase { detail: 'locked'; } @@ -334,28 +345,42 @@ export interface ScriptLocked extends InfoBase { * stale, because one or more output files from the previous run have been * added, removed, or changed. */ -export interface OutputModified extends InfoBase { +export interface OutputModified extends InfoBase { detail: 'output-modified'; } /** * A watch mode iteration started. */ -export interface WatchRunStart extends InfoBase { +export interface WatchRunStart extends InfoBase { detail: 'watch-run-start'; } /** * A watch mode iteration ended. */ -export interface WatchRunEnd extends InfoBase { +export interface WatchRunEnd extends InfoBase { detail: 'watch-run-end'; } +/** + * A service started running. + */ +export interface ServiceStarted extends InfoBase { + detail: 'service-started'; +} + +/** + * A service stopped running. + */ +export interface ServiceStopped extends InfoBase { + detail: 'service-stopped'; +} + /** * A generic info event. */ -export interface GenericInfo extends InfoBase { +export interface GenericInfo extends InfoBase { detail: 'generic'; message: string; } diff --git a/src/execution/service.ts b/src/execution/service.ts new file mode 100644 index 000000000..20311f624 --- /dev/null +++ b/src/execution/service.ts @@ -0,0 +1,40 @@ +/** + * @license + * Copyright 2022 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import {BaseExecution} from './base.js'; +import {Fingerprint} from '../fingerprint.js'; + +import type {ExecutionResult} from './base.js'; +import type {Executor} from '../executor.js'; +import type {ServiceScriptConfig} from '../config.js'; +import type {Logger} from '../logging/logger.js'; + +/** + * Execution for a {@link ServiceScriptConfig}. + */ +export class ServiceScriptExecution extends BaseExecution { + static execute( + script: ServiceScriptConfig, + executor: Executor, + logger: Logger + ): Promise { + return new ServiceScriptExecution(script, executor, logger)._execute(); + } + + private async _execute(): Promise { + const dependencyFingerprints = await this.executeDependencies(); + if (!dependencyFingerprints.ok) { + return dependencyFingerprints; + } + const fingerprint = await Fingerprint.compute( + this.script, + dependencyFingerprints.value + ); + return {ok: true, value: fingerprint}; + } + + // TODO(aomarks) Implement service starting/stopping. +} diff --git a/src/executor.ts b/src/executor.ts index c5a00fcc3..b4a0cdc63 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -6,6 +6,7 @@ import {NoCommandScriptExecution} from './execution/no-command.js'; import {StandardScriptExecution} from './execution/standard.js'; +import {ServiceScriptExecution} from './execution/service.js'; import {ScriptConfig, scriptReferenceToString} from './config.js'; import {WorkerPool} from './util/worker-pool.js'; import {Deferred} from './util/deferred.js'; @@ -134,6 +135,9 @@ export class Executor { if (script.command === undefined) { return NoCommandScriptExecution.execute(script, this, this._logger); } + if (script.service) { + return ServiceScriptExecution.execute(script, this, this._logger); + } return StandardScriptExecution.execute( script, this, diff --git a/src/logging/default-logger.ts b/src/logging/default-logger.ts index 852c338a0..ce08a5cfc 100644 --- a/src/logging/default-logger.ts +++ b/src/logging/default-logger.ts @@ -198,6 +198,11 @@ export class DefaultLogger implements Logger { event.dependency )} which could not be validated. Please file a bug at https://github.com/google/wireit/issues/new, mention this message, that you encountered it in wireit version ${getWireitVersion()}, and give information about your package.json files.` ); + break; + } + case 'service-exited-unexpectedly': { + console.error(`❌${prefix} Service exited unexpectedly`); + break; } } break; @@ -275,6 +280,14 @@ export class DefaultLogger implements Logger { console.log(`ℹ️${prefix} ${event.message}`); break; } + case 'service-started': { + console.log(`⬆️${prefix} Service started`); + break; + } + case 'service-stopped': { + console.log(`⬇️${prefix} Service stopped`); + break; + } } } } diff --git a/src/script-child-process.ts b/src/script-child-process.ts index 05df0b8f3..7f2ccc3ef 100644 --- a/src/script-child-process.ts +++ b/src/script-child-process.ts @@ -10,9 +10,10 @@ import { augmentProcessEnvSafelyIfOnWindows, IS_WINDOWS, } from './util/windows.js'; +import {Deferred} from './util/deferred.js'; import type {Result} from './error.js'; -import type {ScriptConfig} from './config.js'; +import type {ScriptReferenceWithCommand} from './config.js'; import type {ChildProcessWithoutNullStreams} from 'child_process'; import type {ExitNonZero, ExitSignal, SpawnError, Killed} from './event.js'; @@ -44,27 +45,27 @@ export type ScriptChildProcessState = | 'killing' | 'stopped'; -/** - * A script config but the command is required. - */ -type ScriptConfigWithRequiredCommand = ScriptConfig & { - command: Exclude; -}; - /** * A child process spawned during execution of a script. */ export class ScriptChildProcess { - private readonly _script: ScriptConfigWithRequiredCommand; + private readonly _script: ScriptReferenceWithCommand; private readonly _child: ChildProcessWithoutNullStreams; + private readonly _started = new Deferred>(); + private readonly _completed = new Deferred< + Result + >(); private _state: ScriptChildProcessState = 'starting'; + /** + * Resolves when this process starts + */ + readonly started = this._started.promise; + /** * Resolves when this child process ends. */ - readonly completed: Promise< - Result - >; + readonly completed = this._completed.promise; get stdout() { return this._child.stdout; @@ -74,13 +75,21 @@ export class ScriptChildProcess { return this._child.stderr; } - constructor(script: ScriptConfigWithRequiredCommand) { - this._script = script; + constructor(script: ScriptReferenceWithCommand) { + // Copy only the fields we actually require from the script config, because + // the full script config contains references to the full config, which we + // want to allow to be garbage-collected across watch iterations. + this._script = { + packageDir: script.packageDir, + name: script.name, + command: script.command, + extraArgs: script.extraArgs, + }; // TODO(aomarks) Update npm_ environment variables to reflect the new // package. - this._child = spawn(script.command.value, script.extraArgs, { - cwd: script.packageDir, + this._child = spawn(this._script.command.value, this._script.extraArgs, { + cwd: this._script.packageDir, // Conveniently, "shell:true" has the same shell-selection behavior as // "npm run", where on macOS and Linux it is "sh", and on Windows it is // %COMSPEC% || "cmd.exe". @@ -113,94 +122,96 @@ export class ScriptChildProcess { detached: !IS_WINDOWS, }); - this.completed = new Promise((resolve, reject) => { - this._child.on('spawn', () => { - switch (this._state) { - case 'starting': { - this._state = 'started'; - break; - } - case 'killing': { - // We received a kill request while we were still starting. Kill now - // that we're started. - this._actuallyKill(); - break; - } - case 'started': - case 'stopped': { - reject( - new Error( - `Internal error: Expected ScriptChildProcessState ` + - `to be "started" or "killing" but was "${this._state}"` - ) - ); - break; - } - default: { - const never: never = this._state; - reject( - new Error( - `Internal error: unexpected ScriptChildProcessState: ${String( - never - )}` - ) - ); - } + this._child.on('spawn', () => { + switch (this._state) { + case 'starting': { + this._started.resolve({ok: true, value: undefined}); + this._state = 'started'; + break; } - }); + case 'killing': { + this._started.resolve({ok: true, value: undefined}); + // We received a kill request while we were still starting. Kill now + // that we're started. + this._actuallyKill(); + break; + } + case 'started': + case 'stopped': { + const exception = new Error( + `Internal error: Expected ScriptChildProcessState ` + + `to be "started" or "killing" but was "${this._state}"` + ); + this._started.reject(exception); + this._completed.reject(exception); + break; + } + default: { + const never: never = this._state; + const exception = new Error( + `Internal error: unexpected ScriptChildProcessState: ${String( + never + )}` + ); + this._started.reject(exception); + this._completed.reject(exception); + } + } + }); + + this._child.on('error', (error) => { + const result = { + ok: false, + error: { + script, + type: 'failure', + reason: 'spawn-error', + message: error.message, + }, + } as const; + this._started.resolve(result); + this._completed.resolve(result); + this._state = 'stopped'; + }); - this._child.on('error', (error) => { - resolve({ + this._child.on('close', (status, signal) => { + if (this._state === 'killing') { + this._completed.resolve({ ok: false, error: { script, type: 'failure', - reason: 'spawn-error', - message: error.message, + reason: 'killed', }, }); - this._state = 'stopped'; - }); - - this._child.on('close', (status, signal) => { - if (this._state === 'killing') { - resolve({ - ok: false, - error: { - script, - type: 'failure', - reason: 'killed', - }, - }); - } else if (signal !== null) { - resolve({ - ok: false, - error: { - script, - type: 'failure', - reason: 'signal', - signal, - }, - }); - } else if (status !== 0) { - resolve({ - ok: false, - error: { - script, - type: 'failure', - reason: 'exit-non-zero', - // status should only ever be null if signal was not null, but - // this isn't reflected in the TypeScript types. Just in case, and - // to make TypeScript happy, fall back to -1 (which is a - // conventional exit status used for "exited with signal"). - status: status ?? -1, - }, - }); - } else { - resolve({ok: true, value: undefined}); - } - this._state = 'stopped'; - }); + } else if (signal !== null) { + this._completed.resolve({ + ok: false, + error: { + script, + type: 'failure', + reason: 'signal', + signal, + }, + }); + } else if (status !== 0) { + this._completed.resolve({ + ok: false, + error: { + script, + type: 'failure', + reason: 'exit-non-zero', + // status should only ever be null if signal was not null, but + // this isn't reflected in the TypeScript types. Just in case, and + // to make TypeScript happy, fall back to -1 (which is a + // conventional exit status used for "exited with signal"). + status: status ?? -1, + }, + }); + } else { + this._completed.resolve({ok: true, value: undefined}); + } + this._state = 'stopped'; }); } From 35d0980c3a04f56804032bc8f0f9235bdd3905d7 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Wed, 19 Oct 2022 14:21:37 -0700 Subject: [PATCH 03/18] Services: Add service-related analysis properties (#466) 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 https://github.com/google/wireit/issues/33 --- package.json | 50 +++++++++------ src/analyzer.ts | 60 ++++++++++++++--- src/config.ts | 15 +++++ src/test/analysis.test.ts | 131 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 227 insertions(+), 29 deletions(-) create mode 100644 src/test/analysis.test.ts diff --git a/package.json b/package.json index ee3fca9a1..0314587c3 100644 --- a/package.json +++ b/package.json @@ -29,6 +29,7 @@ "format:check": "prettier . -c", "test": "wireit", "test:headless": "wireit", + "test:analysis": "wireit", "test:basic": "wireit", "test:cache-github": "wireit", "test:cache-local": "wireit", @@ -71,6 +72,7 @@ }, "test:headless": { "dependencies": [ + "test:analysis", "test:basic", "test:cache-github", "test:cache-local", @@ -107,8 +109,16 @@ ], "output": [] }, + "test:analysis": { + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^analysis\\.test\\.js$\"", + "dependencies": [ + "build" + ], + "files": [], + "output": [] + }, "test:basic": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"basic\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^basic\\.test\\.js$\"", "dependencies": [ "build" ], @@ -116,7 +126,7 @@ "output": [] }, "test:cache-github": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"cache-github\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^cache-github\\.test\\.js$\"", "dependencies": [ "build" ], @@ -124,7 +134,7 @@ "output": [] }, "test:cache-local": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"cache-local\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^cache-local\\.test\\.js$\"", "dependencies": [ "build" ], @@ -132,7 +142,7 @@ "output": [] }, "test:clean": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"clean\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^clean\\.test\\.js$\"", "dependencies": [ "build" ], @@ -140,7 +150,7 @@ "output": [] }, "test:cli-options": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"cli-options\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^cli-options\\.test\\.js$\"", "dependencies": [ "build" ], @@ -148,7 +158,7 @@ "output": [] }, "test:codeactions": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"codeactions\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^codeactions\\.test\\.js$\"", "dependencies": [ "build" ], @@ -156,7 +166,7 @@ "output": [] }, "test:copy": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"copy\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^copy\\.test\\.js$\"", "dependencies": [ "build" ], @@ -164,7 +174,7 @@ "output": [] }, "test:delete": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"delete\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^delete\\.test\\.js$\"", "dependencies": [ "build" ], @@ -172,7 +182,7 @@ "output": [] }, "test:diagnostic": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"diagnostic\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^diagnostic\\.test\\.js$\"", "dependencies": [ "build" ], @@ -180,7 +190,7 @@ "output": [] }, "test:errors-analysis": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"errors-analysis\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^errors-analysis\\.test\\.js$\"", "dependencies": [ "build" ], @@ -188,7 +198,7 @@ "output": [] }, "test:errors-usage": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"errors-usage\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^errors-usage\\.test\\.js$\"", "dependencies": [ "build" ], @@ -196,7 +206,7 @@ "output": [] }, "test:failures": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"failures\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^failures\\.test\\.js$\"", "dependencies": [ "build" ], @@ -204,7 +214,7 @@ "output": [] }, "test:freshness": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"freshness\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^freshness\\.test\\.js$\"", "dependencies": [ "build" ], @@ -212,7 +222,7 @@ "output": [] }, "test:glob": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"glob\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^glob\\.test\\.js$\"", "dependencies": [ "build" ], @@ -220,7 +230,7 @@ "output": [] }, "test:ide": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"ide\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^ide\\.test\\.js$\"", "dependencies": [ "build" ], @@ -228,7 +238,7 @@ "output": [] }, "test:json-schema": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"json-schema\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^json-schema\\.test\\.js$\"", "dependencies": [ "build" ], @@ -238,7 +248,7 @@ "output": [] }, "test:optimize-mkdirs": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"optimize-mkdirs\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^optimize-mkdirs\\.test\\.js$\"", "dependencies": [ "build" ], @@ -246,7 +256,7 @@ "output": [] }, "test:parallelism": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"parallelism\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^parallelism\\.test\\.js$\"", "dependencies": [ "build" ], @@ -254,7 +264,7 @@ "output": [] }, "test:service": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"service\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^service\\.test\\.js$\"", "dependencies": [ "build" ], @@ -262,7 +272,7 @@ "output": [] }, "test:watch": { - "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"watch\\.test\\.js$\"", + "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^watch\\.test\\.js$\"", "dependencies": [ "build" ], diff --git a/src/analyzer.ts b/src/analyzer.ts index a531fc785..feae56208 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -132,7 +132,11 @@ export class Analyzer { } // We don't care about the result, if there's a cycle error it'll // be added to the scripts' diagnostics. - this._checkForCyclesAndSortDependencies(info.placeholder, new Set()); + this._checkForCyclesAndSortDependencies( + info.placeholder, + new Set(), + true + ); } return this._getDiagnostics(); @@ -196,7 +200,8 @@ export class Analyzer { } const cycleResult = this._checkForCyclesAndSortDependencies( rootConfig, - new Set() + new Set(), + true ); if (!cycleResult.ok) { return { @@ -488,6 +493,7 @@ export class Analyzer { scriptAstNode: scriptCommand, configAstNode: wireitConfig, declaringFile: packageJson.jsonFile, + services: [], }; Object.assign(placeholder, remainingConfig); } @@ -939,7 +945,8 @@ export class Analyzer { */ private _checkForCyclesAndSortDependencies( config: LocallyValidScriptConfig | ScriptConfig | InvalidScriptConfig, - trail: Set + trail: Set, + isDirectlyInvoked: boolean ): Result { if (config.state === 'valid') { // Already validated. @@ -1057,16 +1064,36 @@ export class Analyzer { dependencyStillUnvalidated = dependency.config; continue; } - const result = this._checkForCyclesAndSortDependencies( - dependency.config, - trail - ); - if (!result.ok) { + const validDependencyConfigResult = + this._checkForCyclesAndSortDependencies( + dependency.config, + trail, + // Walk through no-command scripts when determining if something is + // being directly invoked (e.g. if the top-level script has no command + // and simply delegates to one or more other scripts, then those + // dependencies are effectively being directly invoked). + isDirectlyInvoked && config.command === undefined + ); + if (!validDependencyConfigResult.ok) { return { ok: false, - error: this._markAsInvalid(config, result.error.dependencyFailure), + error: this._markAsInvalid( + config, + validDependencyConfigResult.error.dependencyFailure + ), }; } + const validDependencyConfig = validDependencyConfigResult.value; + if (validDependencyConfig.service) { + // We directly depend on a service. + config.services.push(validDependencyConfig); + } else if (validDependencyConfig.command === undefined) { + // We depend on a no-command script, so in effect we depend on all of + // the services it depends on. + for (const service of validDependencyConfig.services) { + config.services.push(service); + } + } } trail.delete(trailKey); } @@ -1101,6 +1128,8 @@ export class Analyzer { // Unfortunately TypeScript doesn't narrow the ...config spread, so we // have to assign explicitly. command: config.command, + isDirectlyInvoked, + serviceConsumers: [], }; } else { validConfig = { @@ -1114,6 +1143,19 @@ export class Analyzer { }; } + // Propagate reverse service dependencies. + if (validConfig.command) { + for (const dependency of validConfig.dependencies) { + if (dependency.config.service) { + dependency.config.serviceConsumers.push(validConfig); + } else if (dependency.config.command === undefined) { + for (const service of dependency.config.services) { + service.serviceConsumers.push(validConfig); + } + } + } + } + // We want to keep the original reference, but get type checking that // the only difference between a ScriptConfig and a // LocallyValidScriptConfig is that the state is 'valid' and the diff --git a/src/config.ts b/src/config.ts index 32e6031fa..2e5aa988e 100644 --- a/src/config.ts +++ b/src/config.ts @@ -80,6 +80,16 @@ export interface ServiceScriptConfig extends BaseScriptConfig, ScriptReferenceWithCommand { service: true; + + /** + * Whether this service is being invoked directly (e.g. `npm run serve`). + */ + isDirectlyInvoked: boolean; + + /** + * Scripts that depend on this service. + */ + serviceConsumers: Array; } /** @@ -97,6 +107,11 @@ interface BaseScriptConfig extends ScriptReference { */ dependencies: Array>; + /** + * The services that need to be started before we can run. + */ + services: Array; + /** * Input file globs for this script. * diff --git a/src/test/analysis.test.ts b/src/test/analysis.test.ts new file mode 100644 index 000000000..9962177c7 --- /dev/null +++ b/src/test/analysis.test.ts @@ -0,0 +1,131 @@ +/** + * @license + * Copyright 2022 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import {suite} from 'uvu'; +import * as assert from 'uvu/assert'; +import {WireitTestRig} from './util/test-rig.js'; +import {Analyzer} from '../analyzer.js'; + +const test = suite<{rig: WireitTestRig}>(); + +test.before.each(async (ctx) => { + try { + ctx.rig = new WireitTestRig(); + await ctx.rig.setup(); + } catch (error) { + // Uvu has a bug where it silently ignores failures in before and after, + // see https://github.com/lukeed/uvu/issues/191. + console.error('uvu before error', error); + process.exit(1); + } +}); + +test.after.each(async (ctx) => { + try { + await ctx.rig.cleanup(); + } catch (error) { + // Uvu has a bug where it silently ignores failures in before and after, + // see https://github.com/lukeed/uvu/issues/191. + console.error('uvu after error', error); + process.exit(1); + } +}); + +test('analyzes services', async ({rig}) => { + // a + // / | \ + // | v v + // | c d + // | / | + // b <-+ | + // v + // e + await rig.write({ + 'package.json': { + scripts: { + a: 'wireit', + b: 'wireit', + c: 'wireit', + d: 'wireit', + e: 'wireit', + }, + wireit: { + a: { + dependencies: ['b', 'c', 'd'], + }, + b: { + command: 'true', + service: true, + }, + c: { + command: 'true', + service: true, + }, + d: { + command: 'true', + dependencies: ['b', 'e'], + }, + e: { + command: 'true', + service: true, + }, + }, + }, + }); + + const analyzer = new Analyzer(); + const result = await analyzer.analyze({packageDir: rig.temp, name: 'a'}, []); + if (!result.config.ok) { + console.log(result.config.error); + throw new Error('Not ok'); + } + + // a + const a = result.config.value; + assert.equal(a.name, 'a'); + if (a.command) { + throw new Error('Expected no-command'); + } + assert.equal(a.dependencies.length, 3); + + // b + const b = a.dependencies[0].config; + assert.equal(b.name, 'b'); + if (!b.service) { + throw new Error('Expected service'); + } + assert.equal(b.serviceConsumers.length, 1); + assert.equal(b.serviceConsumers[0].name, 'd'); + assert.equal(b.isDirectlyInvoked, true); + + // c + const c = a.dependencies[1].config; + assert.equal(c.name, 'c'); + if (!c.service) { + throw new Error('Expected service'); + } + assert.equal(c.isDirectlyInvoked, true); + assert.equal(c.serviceConsumers.length, 0); + assert.equal(c.services.length, 0); + + // d + const d = a.dependencies[2].config; + assert.equal(d.name, 'd'); + assert.equal(d.services.length, 2); + assert.equal(d.services[0].name, 'b'); + assert.equal(d.services[1].name, 'e'); + + // e + const e = d.services[1]; + assert.equal(e.name, 'e'); + if (!e.service) { + throw new Error('Expected service'); + } + assert.equal(e.isDirectlyInvoked, false); + assert.equal(e.serviceConsumers.length, 1); +}); + +test.run(); From 0377bdbbc373ba906b8f25feadd378124466f4f5 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Thu, 20 Oct 2022 16:20:06 -0700 Subject: [PATCH 04/18] Services: Refactoring to surface more Execution API to other scripts (#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 https://github.com/google/wireit/issues/33 --- src/cli.ts | 2 +- src/execution/base.ts | 35 +++++++---- src/execution/no-command.ts | 20 ++---- src/execution/service.ts | 20 +++--- src/execution/standard.ts | 94 ++++++++++++---------------- src/executor.ts | 83 +++++++++++++----------- src/test/cache-github.test.ts | 10 ++- src/test/util/filesystem-test-rig.ts | 26 ++++---- src/test/util/test-rig.ts | 10 +-- src/watcher.ts | 2 +- tsconfig.json | 1 + 11 files changed, 149 insertions(+), 154 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 4ef156c7a..2232e8454 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -100,7 +100,7 @@ const run = async (): Promise> => { options.failureMode, abort ); - const result = await executor.execute(config.value); + const result = await executor.getExecution(config.value).execute(); if (!result.ok) { return result; } diff --git a/src/execution/base.ts b/src/execution/base.ts index 61ea7304e..946411aa3 100644 --- a/src/execution/base.ts +++ b/src/execution/base.ts @@ -29,30 +29,41 @@ export type FailureMode = 'no-new' | 'continue' | 'kill'; * A single execution of a specific script. */ export abstract class BaseExecution { - protected readonly script: T; - protected readonly executor: Executor; - protected readonly logger: Logger; + protected readonly _config: T; + protected readonly _executor: Executor; + protected readonly _logger: Logger; + private _fingerprint?: Promise; - protected constructor(script: T, executor: Executor, logger: Logger) { - this.script = script; - this.executor = executor; - this.logger = logger; + constructor(config: T, executor: Executor, logger: Logger) { + this._config = config; + this._executor = executor; + this._logger = logger; } + /** + * Execute this script and return its fingerprint. Cached, so safe to call + * multiple times. + */ + execute(): Promise { + return (this._fingerprint ??= this._execute()); + } + + protected abstract _execute(): Promise; + /** * Execute all of this script's dependencies. */ - protected async executeDependencies(): Promise< + protected async _executeDependencies(): Promise< Result, Failure[]> > { // Randomize the order we execute dependencies to make it less likely for a // user to inadvertently depend on any specific order, which could indicate // a missing edge in the dependency graph. - shuffle(this.script.dependencies); + shuffle(this._config.dependencies); const dependencyResults = await Promise.all( - this.script.dependencies.map((dependency) => { - return this.executor.execute(dependency.config); + this._config.dependencies.map((dependency) => { + return this._executor.getExecution(dependency.config).execute(); }) ); const results: Array<[ScriptReference, Fingerprint]> = []; @@ -64,7 +75,7 @@ export abstract class BaseExecution { errors.add(error); } } else { - results.push([this.script.dependencies[i].config, result.value]); + results.push([this._config.dependencies[i].config, result.value]); } } if (errors.size > 0) { diff --git a/src/execution/no-command.ts b/src/execution/no-command.ts index b96ace551..5bbe2f502 100644 --- a/src/execution/no-command.ts +++ b/src/execution/no-command.ts @@ -8,33 +8,23 @@ import {BaseExecution} from './base.js'; import {Fingerprint} from '../fingerprint.js'; import type {ExecutionResult} from './base.js'; -import type {Executor} from '../executor.js'; import type {NoCommandScriptConfig} from '../config.js'; -import type {Logger} from '../logging/logger.js'; /** * Execution for a {@link NoCommandScriptConfig}. */ export class NoCommandScriptExecution extends BaseExecution { - static execute( - script: NoCommandScriptConfig, - executor: Executor, - logger: Logger - ): Promise { - return new NoCommandScriptExecution(script, executor, logger)._execute(); - } - - private async _execute(): Promise { - const dependencyFingerprints = await this.executeDependencies(); + protected override async _execute(): Promise { + const dependencyFingerprints = await this._executeDependencies(); if (!dependencyFingerprints.ok) { return dependencyFingerprints; } const fingerprint = await Fingerprint.compute( - this.script, + this._config, dependencyFingerprints.value ); - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'success', reason: 'no-command', }); diff --git a/src/execution/service.ts b/src/execution/service.ts index 20311f624..b2e91dfec 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -8,29 +8,23 @@ import {BaseExecution} from './base.js'; import {Fingerprint} from '../fingerprint.js'; import type {ExecutionResult} from './base.js'; -import type {Executor} from '../executor.js'; import type {ServiceScriptConfig} from '../config.js'; -import type {Logger} from '../logging/logger.js'; /** * Execution for a {@link ServiceScriptConfig}. */ export class ServiceScriptExecution extends BaseExecution { - static execute( - script: ServiceScriptConfig, - executor: Executor, - logger: Logger - ): Promise { - return new ServiceScriptExecution(script, executor, logger)._execute(); - } - - private async _execute(): Promise { - const dependencyFingerprints = await this.executeDependencies(); + /** + * Note `execute` is a bit of a misnomer here, because we don't actually + * execute the command at this stage in the case of services. + */ + protected override async _execute(): Promise { + const dependencyFingerprints = await this._executeDependencies(); if (!dependencyFingerprints.ok) { return dependencyFingerprints; } const fingerprint = await Fingerprint.compute( - this.script, + this._config, dependencyFingerprints.value ); return {ok: true, value: fingerprint}; diff --git a/src/execution/standard.ts b/src/execution/standard.ts index c9ff94d86..adaef1acc 100644 --- a/src/execution/standard.ts +++ b/src/execution/standard.ts @@ -37,34 +37,18 @@ type StandardScriptExecutionState = * Execution for a {@link StandardScriptConfig}. */ export class StandardScriptExecution extends BaseExecution { - static execute( - script: StandardScriptConfig, - executor: Executor, - workerPool: WorkerPool, - cache: Cache | undefined, - logger: Logger - ): Promise { - return new StandardScriptExecution( - script, - executor, - workerPool, - cache, - logger - )._execute(); - } - private _state: StandardScriptExecutionState = 'before-running'; private readonly _cache?: Cache; private readonly _workerPool: WorkerPool; - private constructor( - script: StandardScriptConfig, + constructor( + config: StandardScriptConfig, executor: Executor, workerPool: WorkerPool, cache: Cache | undefined, logger: Logger ) { - super(script, executor, logger); + super(config, executor, logger); this._workerPool = workerPool; this._cache = cache; } @@ -75,10 +59,10 @@ export class StandardScriptExecution extends BaseExecution } } - private async _execute(): Promise { + protected async _execute(): Promise { this._ensureState('before-running'); - const dependencyFingerprints = await this.executeDependencies(); + const dependencyFingerprints = await this._executeDependencies(); if (!dependencyFingerprints.ok) { dependencyFingerprints.error.push(this._startCancelledEvent); return dependencyFingerprints; @@ -95,7 +79,7 @@ export class StandardScriptExecution extends BaseExecution // cache key, because a dependency could create or modify an input file to // this script, which would affect the key. const fingerprint = await Fingerprint.compute( - this.script, + this._config, dependencyFingerprints.value ); if (await this._fingerprintIsFresh(fingerprint)) { @@ -116,7 +100,7 @@ export class StandardScriptExecution extends BaseExecution } const cacheHit = fingerprint.data.fullyTracked - ? await this._cache?.get(this.script, fingerprint) + ? await this._cache?.get(this._config, fingerprint) : undefined; if (this._shouldNotStart) { return {ok: false, error: [this._startCancelledEvent]}; @@ -136,7 +120,7 @@ export class StandardScriptExecution extends BaseExecution * significant amount of time might have elapsed. */ private get _shouldNotStart(): boolean { - return this.executor.shouldStopStartingNewScripts; + return this._executor.shouldStopStartingNewScripts; } /** @@ -144,7 +128,7 @@ export class StandardScriptExecution extends BaseExecution */ private get _startCancelledEvent(): StartCancelled { return { - script: this.script, + script: this._config, type: 'failure', reason: 'start-cancelled', }; @@ -157,7 +141,7 @@ export class StandardScriptExecution extends BaseExecution private async _acquireSystemLockIfNeeded( workFn: () => Promise ): Promise { - if (this.script.output?.values.length === 0) { + if (this._config.output?.values.length === 0) { return workFn(); } @@ -203,8 +187,8 @@ export class StandardScriptExecution extends BaseExecution if ((error as {code: string}).code === 'ELOCKED') { if (!loggedLocked) { // Only log this once. - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'info', detail: 'locked', }); @@ -240,8 +224,8 @@ export class StandardScriptExecution extends BaseExecution * Handle the outcome where the script is already fresh. */ private _handleFresh(fingerprint: Fingerprint): ExecutionResult { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'success', reason: 'fresh', }); @@ -285,8 +269,8 @@ export class StandardScriptExecution extends BaseExecution } await writeFingerprintPromise; - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'success', reason: 'cached', }); @@ -325,8 +309,8 @@ export class StandardScriptExecution extends BaseExecution } this._state = 'running'; - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'info', detail: 'running', }); @@ -334,16 +318,16 @@ export class StandardScriptExecution extends BaseExecution const child = new ScriptChildProcess( // Unfortunately TypeScript doesn't automatically narrow this type // based on the undefined-command check we did just above. - this.script + this._config ); - void this.executor.shouldKillRunningScripts.then(() => { + void this._executor.shouldKillRunningScripts.then(() => { child.kill(); }); child.stdout.on('data', (data: string | Buffer) => { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'output', stream: 'stdout', data, @@ -351,8 +335,8 @@ export class StandardScriptExecution extends BaseExecution }); child.stderr.on('data', (data: string | Buffer) => { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'output', stream: 'stderr', data, @@ -361,8 +345,8 @@ export class StandardScriptExecution extends BaseExecution const result = await child.completed; if (result.ok) { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'success', reason: 'exit-zero', }); @@ -378,7 +362,7 @@ export class StandardScriptExecution extends BaseExecution // By directly notifying the Executor about the failure while we are // still inside the WorkerPool callback, we prevent this race // condition. - this.executor.notifyFailure(); + this._executor.notifyFailure(); } return result; }); @@ -412,7 +396,7 @@ export class StandardScriptExecution extends BaseExecution } private async _shouldClean(fingerprint: Fingerprint) { - const cleanValue = this.script.clean; + const cleanValue = this._config.clean; switch (cleanValue) { case true: { return true; @@ -479,7 +463,7 @@ export class StandardScriptExecution extends BaseExecution if (paths.value === undefined) { return {ok: true, value: undefined}; } - await this._cache.set(this.script, fingerprint, paths.value); + await this._cache.set(this._config, fingerprint, paths.value); return {ok: true, value: undefined}; } @@ -518,14 +502,14 @@ export class StandardScriptExecution extends BaseExecution private async _globOutputFiles(): Promise< Result > { - if (this.script.output === undefined) { + if (this._config.output === undefined) { return {ok: true, value: undefined}; } try { return { ok: true, - value: await glob(this.script.output.values, { - cwd: this.script.packageDir, + value: await glob(this._config.output.values, { + cwd: this._config.packageDir, followSymlinks: false, includeDirectories: true, expandDirectories: true, @@ -542,15 +526,15 @@ export class StandardScriptExecution extends BaseExecution error: { type: 'failure', reason: 'invalid-config-syntax', - script: this.script, + script: this._config, diagnostic: { severity: 'error', message: `Output files must be within the package: ${error.message}`, location: { - file: this.script.declaringFile, + file: this._config.declaringFile, range: { - offset: this.script.output.node.offset, - length: this.script.output.node.length, + offset: this._config.output.node.offset, + length: this._config.output.node.length, }, }, }, @@ -565,7 +549,7 @@ export class StandardScriptExecution extends BaseExecution * Get the directory name where Wireit data can be saved for this script. */ private get _dataDir(): string { - return getScriptDataDir(this.script); + return getScriptDataDir(this._config); } /** @@ -676,8 +660,8 @@ export class StandardScriptExecution extends BaseExecution } const equal = newManifest === oldManifest; if (!equal) { - this.logger.log({ - script: this.script, + this._logger.log({ + script: this._config, type: 'info', detail: 'output-modified', }); diff --git a/src/executor.ts b/src/executor.ts index b4a0cdc63..0edcc73d0 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -7,14 +7,31 @@ import {NoCommandScriptExecution} from './execution/no-command.js'; import {StandardScriptExecution} from './execution/standard.js'; import {ServiceScriptExecution} from './execution/service.js'; -import {ScriptConfig, scriptReferenceToString} from './config.js'; +import {ScriptReferenceString, scriptReferenceToString} from './config.js'; import {WorkerPool} from './util/worker-pool.js'; import {Deferred} from './util/deferred.js'; -import {convertExceptionToFailure} from './error.js'; -import type {ExecutionResult} from './execution/base.js'; import type {Logger} from './logging/logger.js'; import type {Cache} from './caching/cache.js'; +import type { + ScriptConfig, + NoCommandScriptConfig, + ServiceScriptConfig, + StandardScriptConfig, +} from './config.js'; + +type Execution = + | NoCommandScriptExecution + | StandardScriptExecution + | ServiceScriptExecution; + +type ConfigToExecution = T extends NoCommandScriptConfig + ? NoCommandScriptExecution + : T extends StandardScriptConfig + ? StandardScriptExecution + : T extends ServiceScriptConfig + ? ServiceScriptExecution + : never; /** * What to do when a script failure occurs: @@ -30,7 +47,7 @@ export type FailureMode = 'no-new' | 'continue' | 'kill'; * Executes a script that has been analyzed and validated by the Analyzer. */ export class Executor { - private readonly _executions = new Map>(); + private readonly _executions = new Map(); private readonly _logger: Logger; private readonly _workerPool: WorkerPool; private readonly _cache?: Cache; @@ -112,38 +129,32 @@ export class Executor { return this._killRunningScripts.promise; } - async execute(script: ScriptConfig): Promise { - const executionKey = scriptReferenceToString(script); - let promise = this._executions.get(executionKey); - if (promise === undefined) { - promise = this._executeAccordingToKind(script) - .catch((error) => convertExceptionToFailure(error, script)) - .then((result) => { - if (!result.ok) { - this.notifyFailure(); - } - return result; - }); - this._executions.set(executionKey, promise); - } - return promise; - } - - private _executeAccordingToKind( - script: ScriptConfig - ): Promise { - if (script.command === undefined) { - return NoCommandScriptExecution.execute(script, this, this._logger); - } - if (script.service) { - return ServiceScriptExecution.execute(script, this, this._logger); + /** + * Get the execution instance for a script config, creating one if it doesn't + * already exist. + */ + getExecution(config: T): ConfigToExecution { + const key = scriptReferenceToString(config); + let execution = this._executions.get(key); + if (execution === undefined) { + if (config.command === undefined) { + execution = new NoCommandScriptExecution(config, this, this._logger); + } else if (config.service) { + execution = new ServiceScriptExecution(config, this, this._logger); + } else { + execution = new StandardScriptExecution( + config, + this, + this._workerPool, + this._cache, + this._logger + ); + } + this._executions.set(key, execution); } - return StandardScriptExecution.execute( - script, - this, - this._workerPool, - this._cache, - this._logger - ); + // Cast needed because our Map type doesn't know about the config -> + // execution type guarantees. We could make a smarter Map type, but not + // really worth it here. + return execution as ConfigToExecution; } } diff --git a/src/test/cache-github.test.ts b/src/test/cache-github.test.ts index d8060d1ec..531bbd4c5 100644 --- a/src/test/cache-github.test.ts +++ b/src/test/cache-github.test.ts @@ -20,9 +20,13 @@ const __filename = fileURLToPath(import.meta.url); const __dirname = pathlib.dirname(__filename); const repoRoot = pathlib.resolve(__dirname, '..', '..'); -const SELF_SIGNED_CERT = selfsigned.generate([ - {name: 'commonName', value: 'localhost'}, -]); +const SELF_SIGNED_CERT = selfsigned.generate( + [{name: 'commonName', value: 'localhost'}], + // More recent versions of TLS require a larger minimum key size than the + // default of this library (1024). Let's also upgrade from sha1 to sha256 + // while we're at it. + {keySize: 2048, algorithm: 'sha256'} +); const SELF_SIGNED_CERT_PATH = pathlib.resolve( repoRoot, 'temp', diff --git a/src/test/util/filesystem-test-rig.ts b/src/test/util/filesystem-test-rig.ts index 732039aed..bd9d551ba 100644 --- a/src/test/util/filesystem-test-rig.ts +++ b/src/test/util/filesystem-test-rig.ts @@ -21,7 +21,7 @@ export class FilesystemTestRig { readonly temp = pathlib.resolve(repoRoot, 'temp', String(Math.random())); private _state: 'uninitialized' | 'running' | 'done' = 'uninitialized'; - protected assertState(expected: 'uninitialized' | 'running' | 'done') { + protected _assertState(expected: 'uninitialized' | 'running' | 'done') { if (this._state !== expected) { throw new Error( `Expected state to be ${expected} but was ${this._state}` @@ -33,7 +33,7 @@ export class FilesystemTestRig { * Initialize the temporary filesystem. */ async setup() { - this.assertState('uninitialized'); + this._assertState('uninitialized'); this._state = 'running'; await this.mkdir('.'); } @@ -42,7 +42,7 @@ export class FilesystemTestRig { * Delete the temporary filesystem. */ async cleanup(): Promise { - this.assertState('running'); + this._assertState('running'); await this.delete('.'); this._state = 'done'; } @@ -66,7 +66,7 @@ export class FilesystemTestRig { fileOrFiles: string | {[filename: string]: unknown}, data?: string ): Promise { - this.assertState('running'); + this._assertState('running'); if (typeof fileOrFiles === 'string') { const absolute = pathlib.resolve(this.temp, fileOrFiles); await fs.mkdir(pathlib.dirname(absolute), {recursive: true}); @@ -92,7 +92,7 @@ export class FilesystemTestRig { fileOrFiles: string | {[filename: string]: unknown}, data?: string ): Promise { - this.assertState('running'); + this._assertState('running'); if (typeof fileOrFiles === 'string') { const actual = pathlib.resolve(this.temp, fileOrFiles); const temp = actual + '.tmp'; @@ -125,7 +125,7 @@ export class FilesystemTestRig { * Read a file from the temporary filesystem. */ async read(filename: string): Promise { - this.assertState('running'); + this._assertState('running'); return fs.readFile(this.resolve(filename), 'utf8'); } @@ -133,7 +133,7 @@ export class FilesystemTestRig { * Check whether a file exists in the temporary filesystem. */ async exists(filename: string): Promise { - this.assertState('running'); + this._assertState('running'); try { await fs.access(this.resolve(filename)); return true; @@ -149,7 +149,7 @@ export class FilesystemTestRig { * Get filesystem metadata for the given path in the temporary filesystem. */ async lstat(path: string): Promise { - this.assertState('running'); + this._assertState('running'); return fs.lstat(this.resolve(path)); } @@ -158,7 +158,7 @@ export class FilesystemTestRig { * Return false if it is another kind of file, or if it doesn't exit. */ async isDirectory(path: string): Promise { - this.assertState('running'); + this._assertState('running'); try { const stats = await this.lstat(path); return stats.isDirectory(); @@ -176,7 +176,7 @@ export class FilesystemTestRig { * or undefined if it doesn't exist. */ async readlink(path: string): Promise { - this.assertState('running'); + this._assertState('running'); try { return await fs.readlink(this.resolve(path)); } catch (error) { @@ -193,7 +193,7 @@ export class FilesystemTestRig { * directories. */ async mkdir(dirname: string): Promise { - this.assertState('running'); + this._assertState('running'); await fs.mkdir(this.resolve(dirname), {recursive: true}); } @@ -201,7 +201,7 @@ export class FilesystemTestRig { * Delete a file or directory in the temporary filesystem. */ async delete(filename: string): Promise { - this.assertState('running'); + this._assertState('running'); await fs.rm(this.resolve(filename), {force: true, recursive: true}); } @@ -213,7 +213,7 @@ export class FilesystemTestRig { filename: string, windowsType: 'file' | 'dir' | 'junction' ): Promise { - this.assertState('running'); + this._assertState('running'); const absolute = this.resolve(filename); try { await fs.unlink(absolute); diff --git a/src/test/util/test-rig.ts b/src/test/util/test-rig.ts index 0a504b8ca..997bb170e 100644 --- a/src/test/util/test-rig.ts +++ b/src/test/util/test-rig.ts @@ -38,7 +38,7 @@ export class WireitTestRig extends FilesystemTestRig { * Initialize the temporary filesystem, and set up the wireit binary to be * runnable as though it had been installed there through npm. */ - async setup() { + override async setup() { await super.setup(); const absWireitBinaryPath = pathlib.resolve(repoRoot, 'bin', 'wireit.js'); const absWireitTempInstallPath = pathlib.resolve( @@ -78,7 +78,7 @@ export class WireitTestRig extends FilesystemTestRig { binaryPath: string; installPath: string; }) { - this.assertState('running'); + this._assertState('running'); binaryPath = this._resolve(binaryPath); installPath = this._resolve(installPath); @@ -110,7 +110,7 @@ export class WireitTestRig extends FilesystemTestRig { /** * Delete the temporary filesystem and perform other cleanup. */ - async cleanup(): Promise { + override async cleanup(): Promise { await Promise.all(this._commands.map((command) => command.close())); for (const child of this._activeChildProcesses) { child.kill(); @@ -130,7 +130,7 @@ export class WireitTestRig extends FilesystemTestRig { command: string, opts?: {cwd?: string; env?: Record} ): ExecResult { - this.assertState('running'); + this._assertState('running'); const cwd = this._resolve(opts?.cwd ?? '.'); const result = new ExecResult(command, cwd, { // We hard code the parallelism here because by default we infer a value @@ -180,7 +180,7 @@ export class WireitTestRig extends FilesystemTestRig { * Create a new test command. */ async newCommand(): Promise { - this.assertState('running'); + this._assertState('running'); // On Windows, Node IPC is implemented with named pipes, which must be // prefixed by "\\?\pipe\". On Linux/macOS it's a unix domain socket, which // can be any filepath. See https://nodejs.org/api/net.html#ipc-support for diff --git a/src/watcher.ts b/src/watcher.ts index 0283767c4..f5290336e 100644 --- a/src/watcher.ts +++ b/src/watcher.ts @@ -282,7 +282,7 @@ export class Watcher { this._failureMode, this._abort ); - const result = await executor.execute(script); + const result = await executor.getExecution(script).execute(); if (!result.ok) { for (const error of result.error) { this._logger.log(error); diff --git a/tsconfig.json b/tsconfig.json index b7b3ced3e..b3b5b8a87 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -15,6 +15,7 @@ "forceConsistentCasingInFileNames": true, "allowSyntheticDefaultImports": true, "useUnknownInCatchVariables": true, + "noImplicitOverride": true, "incremental": true, "tsBuildInfoFile": ".tsbuildinfo", "composite": true From d8e3e769d95eee2504f930f9c9f5c2a0c5ce24c6 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 21 Oct 2022 10:46:06 -0700 Subject: [PATCH 05/18] Services: Track when services are not needed (#473) 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 https://github.com/google/wireit/issues/33 --- src/execution/base.ts | 19 +++++++ src/execution/service.ts | 16 +++++- src/execution/standard.ts | 102 +++++++++++++++++++++----------------- src/executor.ts | 9 +++- src/watcher.ts | 4 +- 5 files changed, 100 insertions(+), 50 deletions(-) diff --git a/src/execution/base.ts b/src/execution/base.ts index 946411aa3..13647b0e5 100644 --- a/src/execution/base.ts +++ b/src/execution/base.ts @@ -6,6 +6,7 @@ import {shuffle} from '../util/shuffle.js'; import {Fingerprint} from '../fingerprint.js'; +import {Deferred} from '../util/deferred.js'; import type {Result} from '../error.js'; import type {Executor} from '../executor.js'; @@ -84,3 +85,21 @@ export abstract class BaseExecution { return {ok: true, value: results}; } } + +/** + * A single execution of a specific script which has a command. + */ +export abstract class BaseExecutionWithCommand< + T extends ScriptConfig & { + command: Exclude; + } +> extends BaseExecution { + protected readonly _servicesNotNeeded = new Deferred(); + + /** + * Resolves when this script no longer needs any of its service dependencies + * to be running. This could happen because it finished, failed, or never + * needed to run at all. + */ + readonly servicesNotNeeded = this._servicesNotNeeded.promise; +} diff --git a/src/execution/service.ts b/src/execution/service.ts index b2e91dfec..499d6885c 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -4,16 +4,28 @@ * SPDX-License-Identifier: Apache-2.0 */ -import {BaseExecution} from './base.js'; +import {BaseExecutionWithCommand} from './base.js'; import {Fingerprint} from '../fingerprint.js'; import type {ExecutionResult} from './base.js'; import type {ServiceScriptConfig} from '../config.js'; +import type {Executor} from '../executor.js'; +import type {Logger} from '../logging/logger.js'; /** * Execution for a {@link ServiceScriptConfig}. */ -export class ServiceScriptExecution extends BaseExecution { +export class ServiceScriptExecution extends BaseExecutionWithCommand { + constructor( + config: ServiceScriptConfig, + executor: Executor, + logger: Logger, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + _abort: Promise + ) { + super(config, executor, logger); + } + /** * Note `execute` is a bit of a misnomer here, because we don't actually * execute the command at this stage in the case of services. diff --git a/src/execution/standard.ts b/src/execution/standard.ts index adaef1acc..920303d44 100644 --- a/src/execution/standard.ts +++ b/src/execution/standard.ts @@ -13,7 +13,7 @@ import {glob, GlobOutsideCwdError} from '../util/glob.js'; import {deleteEntries} from '../util/delete.js'; import lockfile from 'proper-lockfile'; import {ScriptChildProcess} from '../script-child-process.js'; -import {BaseExecution} from './base.js'; +import {BaseExecutionWithCommand} from './base.js'; import {Fingerprint} from '../fingerprint.js'; import {computeManifestEntry} from '../util/manifest.js'; @@ -36,7 +36,7 @@ type StandardScriptExecutionState = /** * Execution for a {@link StandardScriptConfig}. */ -export class StandardScriptExecution extends BaseExecution { +export class StandardScriptExecution extends BaseExecutionWithCommand { private _state: StandardScriptExecutionState = 'before-running'; private readonly _cache?: Cache; private readonly _workerPool: WorkerPool; @@ -60,57 +60,61 @@ export class StandardScriptExecution extends BaseExecution } protected async _execute(): Promise { - this._ensureState('before-running'); - - const dependencyFingerprints = await this._executeDependencies(); - if (!dependencyFingerprints.ok) { - dependencyFingerprints.error.push(this._startCancelledEvent); - return dependencyFingerprints; - } - - // Significant time could have elapsed since we last checked because our - // dependencies had to finish. - if (this._shouldNotStart) { - return {ok: false, error: [this._startCancelledEvent]}; - } + try { + this._ensureState('before-running'); - return this._acquireSystemLockIfNeeded(async () => { - // Note we must wait for dependencies to finish before generating the - // cache key, because a dependency could create or modify an input file to - // this script, which would affect the key. - const fingerprint = await Fingerprint.compute( - this._config, - dependencyFingerprints.value - ); - if (await this._fingerprintIsFresh(fingerprint)) { - const manifestFresh = await this._outputManifestIsFresh(); - if (!manifestFresh.ok) { - return {ok: false, error: [manifestFresh.error]}; - } - if (manifestFresh.value) { - return this._handleFresh(fingerprint); - } + const dependencyFingerprints = await this._executeDependencies(); + if (!dependencyFingerprints.ok) { + dependencyFingerprints.error.push(this._startCancelledEvent); + return dependencyFingerprints; } - // Computing the fingerprint can take some time, and the next operation is - // destructive. Another good opportunity to check if we should still - // start. + // Significant time could have elapsed since we last checked because our + // dependencies had to finish. if (this._shouldNotStart) { return {ok: false, error: [this._startCancelledEvent]}; } - const cacheHit = fingerprint.data.fullyTracked - ? await this._cache?.get(this._config, fingerprint) - : undefined; - if (this._shouldNotStart) { - return {ok: false, error: [this._startCancelledEvent]}; - } - if (cacheHit !== undefined) { - return this._handleCacheHit(cacheHit, fingerprint); - } + return this._acquireSystemLockIfNeeded(async () => { + // Note we must wait for dependencies to finish before generating the + // cache key, because a dependency could create or modify an input file to + // this script, which would affect the key. + const fingerprint = await Fingerprint.compute( + this._config, + dependencyFingerprints.value + ); + if (await this._fingerprintIsFresh(fingerprint)) { + const manifestFresh = await this._outputManifestIsFresh(); + if (!manifestFresh.ok) { + return {ok: false, error: [manifestFresh.error]}; + } + if (manifestFresh.value) { + return this._handleFresh(fingerprint); + } + } - return this._handleNeedsRun(fingerprint); - }); + // Computing the fingerprint can take some time, and the next operation is + // destructive. Another good opportunity to check if we should still + // start. + if (this._shouldNotStart) { + return {ok: false, error: [this._startCancelledEvent]}; + } + + const cacheHit = fingerprint.data.fullyTracked + ? await this._cache?.get(this._config, fingerprint) + : undefined; + if (this._shouldNotStart) { + return {ok: false, error: [this._startCancelledEvent]}; + } + if (cacheHit !== undefined) { + return this._handleCacheHit(cacheHit, fingerprint); + } + + return this._handleNeedsRun(fingerprint); + }); + } finally { + this._servicesNotNeeded.resolve(); + } } /** @@ -239,6 +243,10 @@ export class StandardScriptExecution extends BaseExecution cacheHit: CacheHit, fingerprint: Fingerprint ): Promise { + // Optimization: early signal that services are not needed while we're still + // restoring from cache. + this._servicesNotNeeded.resolve(); + // Delete the fingerprint and other files. It's important we do this before // restoring from cache, because we don't want to think that the previous // fingerprint is still valid when it no longer is. @@ -373,6 +381,10 @@ export class StandardScriptExecution extends BaseExecution return {ok: false, error: [childResult.error]}; } + // Optimization: early signal that services are no longer needed while we're + // still writing the fingerprint file etc. + this._servicesNotNeeded.resolve(); + const writeFingerprintPromise = this._writeFingerprintFile(fingerprint); const outputFilesAfterRunning = await this._globOutputFilesAfterRunning(); if (!outputFilesAfterRunning.ok) { diff --git a/src/executor.ts b/src/executor.ts index 0edcc73d0..b800a2dcf 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -51,6 +51,7 @@ export class Executor { private readonly _logger: Logger; private readonly _workerPool: WorkerPool; private readonly _cache?: Cache; + private readonly _abort: Deferred; /** Resolves when the first failure occurs in any script. */ private readonly _failureOccured = new Deferred(); @@ -69,6 +70,7 @@ export class Executor { this._logger = logger; this._workerPool = workerPool; this._cache = cache; + this._abort = abort; // If this entire execution is aborted because e.g. the user sent a SIGINT // to the Wireit process, then dont start new scripts, and kill running @@ -140,7 +142,12 @@ export class Executor { if (config.command === undefined) { execution = new NoCommandScriptExecution(config, this, this._logger); } else if (config.service) { - execution = new ServiceScriptExecution(config, this, this._logger); + execution = new ServiceScriptExecution( + config, + this, + this._logger, + this._abort.promise + ); } else { execution = new StandardScriptExecution( config, diff --git a/src/watcher.ts b/src/watcher.ts index f5290336e..af18a461b 100644 --- a/src/watcher.ts +++ b/src/watcher.ts @@ -48,11 +48,11 @@ type WatcherState = | 'aborted'; function unknownState(state: never) { - throw new Error(`Unknown watcher state ${String(state)}`); + return new Error(`Unknown watcher state ${String(state)}`); } function unexpectedState(state: WatcherState) { - throw new Error(`Unexpected watcher state ${state}`); + return new Error(`Unexpected watcher state ${state}`); } /** From 0566c9518a43ec435211948f68eed74b27b29bce Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 21 Oct 2022 14:08:38 -0700 Subject: [PATCH 06/18] Services: Integrate into standard scripts (#474) 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 https://github.com/google/wireit/issues/33 --- src/execution/base.ts | 33 ++++++++++++++++++++++ src/execution/service.ts | 23 +++++++++++++++- src/execution/standard.ts | 58 ++++++++++++++++++++++++++++++++++----- 3 files changed, 106 insertions(+), 8 deletions(-) diff --git a/src/execution/base.ts b/src/execution/base.ts index 13647b0e5..d47b622d1 100644 --- a/src/execution/base.ts +++ b/src/execution/base.ts @@ -102,4 +102,37 @@ export abstract class BaseExecutionWithCommand< * needed to run at all. */ readonly servicesNotNeeded = this._servicesNotNeeded.promise; + + /** + * Resolves when any of the services this script depends on have terminated + * (see {@link ServiceScriptExecution.terminated} for exact definiton). + */ + readonly anyServiceTerminated = Promise.race( + this._config.services.map( + (service) => this._executor.getExecution(service).terminated + ) + ); + + /** + * Ensure that all of the services this script depends on are running. + */ + protected async _startServices(): Promise> { + if (this._config.services.length > 0) { + const results = await Promise.all( + this._config.services.map((service) => + this._executor.getExecution(service).start() + ) + ); + const errors: Failure[] = []; + for (const result of results) { + if (!result.ok) { + errors.push(...result.error); + } + } + if (errors.length > 0) { + return {ok: false, error: errors}; + } + } + return {ok: true, value: undefined}; + } } diff --git a/src/execution/service.ts b/src/execution/service.ts index 499d6885c..6323ca6cd 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -6,16 +6,31 @@ import {BaseExecutionWithCommand} from './base.js'; import {Fingerprint} from '../fingerprint.js'; +import {Deferred} from '../util/deferred.js'; import type {ExecutionResult} from './base.js'; import type {ServiceScriptConfig} from '../config.js'; import type {Executor} from '../executor.js'; import type {Logger} from '../logging/logger.js'; +import type {Failure} from '../event.js'; +import type {Result} from '../error.js'; /** * Execution for a {@link ServiceScriptConfig}. */ export class ServiceScriptExecution extends BaseExecutionWithCommand { + private readonly _terminated = new Deferred>(); + + /** + * Resolves as "ok" when this script decides it is no longer needed, and + * either has begun shutting down, or never needed to start in the first + * place. + * + * Resolves with an error if this service exited unexpectedly, or if any of + * its own service dependencies exited unexpectedly. + */ + readonly terminated = this._terminated.promise; + constructor( config: ServiceScriptConfig, executor: Executor, @@ -42,5 +57,11 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand> { + // TODO(aomarks) Implement service starting/stopping. + throw new Error('Not implemented'); + } } diff --git a/src/execution/standard.ts b/src/execution/standard.ts index 920303d44..04904d1af 100644 --- a/src/execution/standard.ts +++ b/src/execution/standard.ts @@ -24,7 +24,7 @@ import type {StandardScriptConfig} from '../config.js'; import type {FingerprintString} from '../fingerprint.js'; import type {Logger} from '../logging/logger.js'; import type {Cache, CacheHit} from '../caching/cache.js'; -import type {StartCancelled} from '../event.js'; +import type {Failure, StartCancelled} from '../event.js'; import type {AbsoluteEntry} from '../util/glob.js'; import type {FileManifestEntry, FileManifestString} from '../util/manifest.js'; @@ -316,6 +316,41 @@ export class StandardScriptExecution extends BaseExecutionWithCommand 0) { + const servicesStarted = await this._startServices(); + if (!servicesStarted.ok) { + return servicesStarted; + } + + void this.anyServiceTerminated.then((result) => { + if (this._state === 'after-running') { + // This is expected after we're done. + return; + } + if (result.ok) { + // This should never happen and indicates an internal error. The + // service believed that nothing was depending on it anymore, but + // we're still running. + earlyServiceTermination = { + script: this._config, + type: 'failure', + reason: 'unknown-error-thrown', + error: new Error( + 'Internal error: service dependency terminated unexpectedly' + ), + }; + } else { + // The service knows it exited too early. Propagate that error. + earlyServiceTermination = result.error; + } + // Stop running. If a service we depend on is down, then we know we're + // in an invalid state too. + child.kill(); + this._executor.notifyFailure(); + }); + } + this._state = 'running'; this._logger.log({ script: this._config, @@ -353,11 +388,15 @@ export class StandardScriptExecution extends BaseExecutionWithCommand Date: Sat, 22 Oct 2022 12:14:20 -0700 Subject: [PATCH 07/18] Services: State diagram (#475) Adds a state diagram as documentation for services. Part of https://github.com/google/wireit/issues/33 --- src/execution/service.ts | 63 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/src/execution/service.ts b/src/execution/service.ts index 6323ca6cd..2fdf3bc83 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -17,6 +17,69 @@ import type {Result} from '../error.js'; /** * Execution for a {@link ServiceScriptConfig}. + * + * Note that this class represents a service _bound to one particular execution_ + * of the script graph. In non-watch mode (`npm run ...`), there will be one + * instance of this class per service. In watch mode (`npm run --watch ...`), + * there will be one instance of this class per service _per watch iteration_, + * and the underlying child process will be transfered between instances of this + * class whenever possible to avoid restarts. + * + * ``` + * ┌─────────┐ + * ╭─◄─ abort ────┤ INITIAL │ + * │ └────┬────┘ + * │ │ + * ▼ execute + * │ │ + * │ ┌───────▼────────┐ + * ├─◄─ abort ─┤ FINGERPRINTING ├──── depExecErr ────►───╮ + * │ └───────┬────────┘ │ + * │ │ │ + * ▼ fingerprinted │ + * │ │ │ + * │ ┌─────▼─────┐ │ + * ├─◄─ abort ───┤ UNSTARTED │ │ + * │ └─────┬─────┘ ▼ + * │ │ │ + * │ start │ + * │ │ ╭─╮ │ + * │ │ │ start │ + * │ ┌────▼──▼─┴┐ │ + * │ ╭◄─ abort ┤ STARTING ├─── startErr or ────►──────┤ + * │ │ └────┬────┬┘ depServiceStartErr │ + * ▼ │ │ │ │ + * │ │ │ ▼ │ + * │ │ │ ╰─── depServiceExit ──►──╮ │ + * │ │ started │ │ + * │ ▼ │ ╭─╮ ▼ │ + * │ │ │ │ start │ │ + * │ │ ┌────▼─▼─┴┐ │ │ + * │ ├◄─ abort ┤ STARTED ├── exit ─────────────►──│───┤ + * │ │ └────┬─┬─┬┘ │ │ + * │ │ │ │ ╰─── detach ──╮ │ │ + * │ │ │ ▼ │ │ │ + * │ │ │ ╰───── depServiceExit ───►──┤ │ + * │ │ │ │ │ │ + * │ │ allConsumersDone │ │ │ + * │ ▼ (unless directly invoked) │ │ │ + * │ │ │ ▼ ▼ ▼ + * ▼ │ │ ╭─╮ │ │ │ + * │ │ │ │ start │ │ │ + * │ │ ┌────▼──▼─┴┐ │ │ │ + * │ ╰─────────► STOPPING ◄─────────────◄─────────╯ │ + * │ └┬─▲─┬─────┘ │ │ + * │ abort │ │ │ │ + * │ ╰─╯ │ │ │ + * │ exit │ │ + * │ │ ╭─╮ │ │ ╭─╮ + * │ │ │ start │ │ │ start + * │ ┌────▼─▼─┴┐ ┌────▼─────┐ ┌───▼─▼─┴┐ + * ╰──────────────► STOPPED │ │ DETACHED │ │ FAILED │ + * └┬─▲──────┘ └┬─▲───────┘ └┬─▲─────┘ + * abort │ *all* │ abort │ + * ╰─╯ ╰─╯ ╰─╯ + * ``` */ export class ServiceScriptExecution extends BaseExecutionWithCommand { private readonly _terminated = new Deferred>(); From 1a1f7b52c77c7e4d18f1ed51d0dc8c055a8b347d Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Mon, 24 Oct 2022 11:38:02 -0700 Subject: [PATCH 08/18] Services: Support for very basic consumer/service test case (#476) 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 https://github.com/google/wireit/issues/33 --- src/execution/service.ts | 273 ++++++++++++++++++++++++++++-- src/execution/standard.ts | 2 +- src/test/service.test.ts | 54 ++++++ src/test/util/test-rig-command.ts | 7 + 4 files changed, 322 insertions(+), 14 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index 2fdf3bc83..1cf5924de 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -7,14 +7,48 @@ import {BaseExecutionWithCommand} from './base.js'; import {Fingerprint} from '../fingerprint.js'; import {Deferred} from '../util/deferred.js'; +import {ScriptChildProcess} from '../script-child-process.js'; import type {ExecutionResult} from './base.js'; -import type {ServiceScriptConfig} from '../config.js'; +import type {ScriptReference, ServiceScriptConfig} from '../config.js'; import type {Executor} from '../executor.js'; import type {Logger} from '../logging/logger.js'; import type {Failure} from '../event.js'; import type {Result} from '../error.js'; +type ServiceState = + | {id: 'initial'} + | { + id: 'executingDeps'; + fingerprint: Deferred; + } + | { + id: 'fingerprinting'; + fingerprint: Deferred; + } + | {id: 'unstarted'} + | { + id: 'starting'; + child: ScriptChildProcess; + started: Deferred>; + } + | { + id: 'started'; + child: ScriptChildProcess; + } + | {id: 'stopping'} + | {id: 'stopped'}; + +function unknownState(state: never) { + return new Error( + `Unknown service state ${String((state as ServiceState).id)}` + ); +} + +function unexpectedState(state: ServiceState) { + return new Error(`Unexpected service state ${state.id}`); +} + /** * Execution for a {@link ServiceScriptConfig}. * @@ -33,7 +67,13 @@ import type {Result} from '../error.js'; * ▼ execute * │ │ * │ ┌───────▼────────┐ - * ├─◄─ abort ─┤ FINGERPRINTING ├──── depExecErr ────►───╮ + * ├─◄─ abort ─┤ EXECUTING_DEPS ├──── depExecErr ────►───╮ + * │ └───────┬────────┘ │ + * │ │ │ + * ▼ depsExecuted │ + * │ │ │ + * │ ┌───────▼────────┐ │ + * ├─◄─ abort ─┤ FINGERPRINTING │ │ * │ └───────┬────────┘ │ * │ │ │ * ▼ fingerprinted │ @@ -82,6 +122,7 @@ import type {Result} from '../error.js'; * ``` */ export class ServiceScriptExecution extends BaseExecutionWithCommand { + private _state: ServiceState = {id: 'initial'}; private readonly _terminated = new Deferred>(); /** @@ -108,23 +149,229 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { - const dependencyFingerprints = await this._executeDependencies(); - if (!dependencyFingerprints.ok) { - return dependencyFingerprints; + protected override _execute(): Promise { + switch (this._state.id) { + case 'initial': { + this._state = { + id: 'executingDeps', + fingerprint: new Deferred(), + }; + void this._executeDependencies().then((result) => { + if (result.ok) { + this._onDepsExecuted(result.value); + } else { + this._onDepExecErr(result); + } + }); + return this._state.fingerprint.promise; + } + case 'executingDeps': + case 'fingerprinting': + case 'unstarted': + case 'starting': + case 'started': + case 'stopping': + case 'stopped': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + + private _onDepsExecuted( + depFingerprints: Array<[ScriptReference, Fingerprint]> + ): void { + switch (this._state.id) { + case 'executingDeps': { + this._state = { + id: 'fingerprinting', + fingerprint: this._state.fingerprint, + }; + void Fingerprint.compute(this._config, depFingerprints).then( + (result) => { + this._onFingerprinted(result); + } + ); + return; + } + case 'initial': + case 'fingerprinting': + case 'unstarted': + case 'starting': + case 'started': + case 'stopping': + case 'stopped': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + + private _onDepExecErr(result: ExecutionResult & {ok: false}) { + switch (this._state.id) { + case 'executingDeps': { + this._state.fingerprint.resolve(result); + return; + } + case 'initial': + case 'fingerprinting': + case 'unstarted': + case 'starting': + case 'started': + case 'stopping': + case 'stopped': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + + private _onFingerprinted(fingerprint: Fingerprint) { + switch (this._state.id) { + case 'fingerprinting': { + this._state.fingerprint.resolve({ok: true, value: fingerprint}); + this._state = {id: 'unstarted'}; + return; + } + case 'initial': + case 'executingDeps': + case 'unstarted': + case 'starting': + case 'started': + case 'stopping': + case 'stopped': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } } - const fingerprint = await Fingerprint.compute( - this._config, - dependencyFingerprints.value - ); - return {ok: true, value: fingerprint}; } /** * Start this service if it isn't already started. */ start(): Promise> { - // TODO(aomarks) Implement service starting/stopping. - throw new Error('Not implemented'); + switch (this._state.id) { + case 'unstarted': { + this._state = { + id: 'starting', + child: new ScriptChildProcess(this._config), + started: new Deferred(), + }; + void this._state.child.started.then(() => { + this._onChildStarted(); + }); + void this._state.child.completed.then(() => { + this._onChildExited(); + }); + return this._state.started.promise; + } + case 'initial': + case 'executingDeps': + case 'fingerprinting': + case 'starting': + case 'started': + case 'stopping': + case 'stopped': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + + private _onChildStarted() { + switch (this._state.id) { + case 'starting': { + this._state.started.resolve({ok: true, value: undefined}); + this._logger.log({ + script: this._config, + type: 'info', + detail: 'service-started', + }); + this._state = { + id: 'started', + child: this._state.child, + }; + const allConsumersDone = Promise.all( + this._config.serviceConsumers.map( + (consumer) => + this._executor.getExecution(consumer).servicesNotNeeded + ) + ); + void allConsumersDone.then(() => { + this._allConsumersDone(); + }); + return; + } + case 'initial': + case 'executingDeps': + case 'fingerprinting': + case 'unstarted': + case 'started': + case 'stopping': + case 'stopped': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + + private _allConsumersDone() { + switch (this._state.id) { + case 'started': { + this._state.child.kill(); + this._state = {id: 'stopping'}; + return; + } + case 'initial': + case 'executingDeps': + case 'fingerprinting': + case 'unstarted': + case 'starting': + case 'stopping': + case 'stopped': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + + private _onChildExited() { + switch (this._state.id) { + case 'stopping': { + this._state = {id: 'stopped'}; + this._logger.log({ + script: this._config, + type: 'info', + detail: 'service-stopped', + }); + return; + } + case 'initial': + case 'executingDeps': + case 'fingerprinting': + case 'unstarted': + case 'starting': + case 'started': + case 'stopped': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } } } diff --git a/src/execution/standard.ts b/src/execution/standard.ts index 04904d1af..f73e6939c 100644 --- a/src/execution/standard.ts +++ b/src/execution/standard.ts @@ -75,7 +75,7 @@ export class StandardScriptExecution extends BaseExecutionWithCommand { + return await this._acquireSystemLockIfNeeded(async () => { // Note we must wait for dependencies to finish before generating the // cache key, because a dependency could create or modify an input file to // this script, which would affect the key. diff --git a/src/test/service.test.ts b/src/test/service.test.ts index fb42dd613..1ed8eced2 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -5,6 +5,8 @@ */ import {suite} from 'uvu'; +import * as assert from 'uvu/assert'; +import {timeout} from './util/uvu-timeout.js'; import {WireitTestRig} from './util/test-rig.js'; const test = suite<{rig: WireitTestRig}>(); @@ -32,4 +34,56 @@ test.after.each(async (ctx) => { } }); +test( + 'simple consumer and service', + timeout(async ({rig}) => { + // consumer + // | + // v + // service + + const consumer = await rig.newCommand(); + const service = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + consumer: 'wireit', + service: 'wireit', + }, + wireit: { + consumer: { + command: consumer.command, + dependencies: ['service'], + }, + service: { + command: service.command, + service: true, + }, + }, + }, + }); + + const wireit = rig.exec('npm run consumer'); + + // The service starts because the consumer depends on it + const serviceInv = await service.nextInvocation(); + await wireit.waitForLog(/Service started/); + + // The consumer starts and finishes + const consumerInv = await consumer.nextInvocation(); + // Wait a moment to ensure the service stays running + await new Promise((resolve) => setTimeout(resolve, 100)); + assert.ok(serviceInv.isRunning); + consumerInv.exit(0); + + // The service stops because the consumer is done + await serviceInv.closed; + await wireit.waitForLog(/Service stopped/); + + await wireit.exit; + assert.equal(service.numInvocations, 1); + assert.equal(consumer.numInvocations, 1); + }) +); + test.run(); diff --git a/src/test/util/test-rig-command.ts b/src/test/util/test-rig-command.ts index 30d6ae1b2..764c8bbd4 100644 --- a/src/test/util/test-rig-command.ts +++ b/src/test/util/test-rig-command.ts @@ -194,6 +194,13 @@ export class WireitTestRigCommandInvocation extends IpcClient< return this._closed.promise; } + /** + * Return whether this invocation is still running. + */ + get isRunning(): boolean { + return this._state === 'connected'; + } + /** * Tell this invocation to exit with the given code. */ From 81af6b35c346d4b23c0657536d80caae56de800d Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Wed, 26 Oct 2022 09:14:45 -0700 Subject: [PATCH 09/18] Services: Stdout/stderr and various error case handling (#483) 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 https://github.com/google/wireit/issues/33 --- src/cli.ts | 3 +- src/execution/base.ts | 2 +- src/execution/service.ts | 257 +++++++++++++++++---- src/execution/standard.ts | 2 +- src/executor.ts | 20 ++ src/test/service.test.ts | 292 +++++++++++++++++++++++- src/test/util/test-rig-command-child.ts | 8 + src/test/util/test-rig.ts | 52 +++-- src/watcher.ts | 3 +- 9 files changed, 574 insertions(+), 65 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 2232e8454..c50304fe9 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -94,13 +94,14 @@ const run = async (): Promise> => { return config; } const executor = new Executor( + config.value, logger, workerPool, cache, options.failureMode, abort ); - const result = await executor.getExecution(config.value).execute(); + const result = await executor.execute(); if (!result.ok) { return result; } diff --git a/src/execution/base.ts b/src/execution/base.ts index d47b622d1..dfdd6a89e 100644 --- a/src/execution/base.ts +++ b/src/execution/base.ts @@ -107,7 +107,7 @@ export abstract class BaseExecutionWithCommand< * Resolves when any of the services this script depends on have terminated * (see {@link ServiceScriptExecution.terminated} for exact definiton). */ - readonly anyServiceTerminated = Promise.race( + protected readonly _anyServiceTerminated = Promise.race( this._config.services.map( (service) => this._executor.getExecution(service).terminated ) diff --git a/src/execution/service.ts b/src/execution/service.ts index 1cf5924de..9a3bcdaa4 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -27,6 +27,10 @@ type ServiceState = fingerprint: Deferred; } | {id: 'unstarted'} + | { + id: 'depsStarting'; + started: Deferred>; + } | { id: 'starting'; child: ScriptChildProcess; @@ -37,7 +41,15 @@ type ServiceState = child: ScriptChildProcess; } | {id: 'stopping'} - | {id: 'stopped'}; + | {id: 'stopped'} + | { + id: 'failing'; + failure: Failure; + } + | { + id: 'failed'; + failure: Failure; + }; function unknownState(state: never) { return new Error( @@ -76,45 +88,54 @@ function unexpectedState(state: ServiceState) { * ├─◄─ abort ─┤ FINGERPRINTING │ │ * │ └───────┬────────┘ │ * │ │ │ - * ▼ fingerprinted │ + * ▼ fingerprinted ▼ * │ │ │ * │ ┌─────▼─────┐ │ * ├─◄─ abort ───┤ UNSTARTED │ │ - * │ └─────┬─────┘ ▼ + * │ └─────┬─────┘ │ * │ │ │ - * │ start │ + * │ start ╭─╮ │ + * │ │ │ start │ + * │ ┌───────▼────▼─┴┐ │ + * ├─◄─ abort ─┤ DEPS_STARTING ├───── depStartErr ───►───┤ + * │ └───────┬───────┘ │ + * │ │ │ + * │ depsStarted ▼ * │ │ ╭─╮ │ * │ │ │ start │ * │ ┌────▼──▼─┴┐ │ - * │ ╭◄─ abort ┤ STARTING ├─── startErr or ────►──────┤ - * │ │ └────┬────┬┘ depServiceStartErr │ - * ▼ │ │ │ │ - * │ │ │ ▼ │ - * │ │ │ ╰─── depServiceExit ──►──╮ │ - * │ │ started │ │ - * │ ▼ │ ╭─╮ ▼ │ - * │ │ │ │ start │ │ - * │ │ ┌────▼─▼─┴┐ │ │ - * │ ├◄─ abort ┤ STARTED ├── exit ─────────────►──│───┤ - * │ │ └────┬─┬─┬┘ │ │ - * │ │ │ │ ╰─── detach ──╮ │ │ - * │ │ │ ▼ │ │ │ - * │ │ │ ╰───── depServiceExit ───►──┤ │ - * │ │ │ │ │ │ - * │ │ allConsumersDone │ │ │ - * │ ▼ (unless directly invoked) │ │ │ - * │ │ │ ▼ ▼ ▼ - * ▼ │ │ ╭─╮ │ │ │ - * │ │ │ │ start │ │ │ - * │ │ ┌────▼──▼─┴┐ │ │ │ - * │ ╰─────────► STOPPING ◄─────────────◄─────────╯ │ - * │ └┬─▲─┬─────┘ │ │ - * │ abort │ │ │ │ - * │ ╰─╯ │ │ │ - * │ exit │ │ - * │ │ ╭─╮ │ │ ╭─╮ - * │ │ │ start │ │ │ start - * │ ┌────▼─▼─┴┐ ┌────▼─────┐ ┌───▼─▼─┴┐ + * │ ╭◄─ abort ┤ STARTING ├──── startErr ──────►──────┤ + * │ │ └────┬────┬┘ │ + * │ │ │ │ │ + * │ │ │ ╰─ depServiceExit ─►─╮ │ + * ▼ │ │ │ │ + * │ │ │ │ │ + * │ ▼ │ ▼ ▼ + * │ │ started │ │ + * │ │ │ ╭─╮ │ │ + * │ │ │ │ start │ │ + * │ │ ┌────▼─▼─┴┐ │ │ + * │ ├◄─ abort ┤ STARTED ├── exit ────────────────────┤ + * │ │ └────┬─┬─┬┘ │ │ + * │ │ │ │ │ │ │ + * │ │ │ │ ╰── depServiceExit ─►─┤ │ + * │ │ │ │ │ │ + * │ │ │ ╰───── detach ──╮ │ │ + * │ ▼ │ │ │ │ + * │ │ allConsumersDone │ │ │ + * │ │ (unless directly invoked) │ │ │ + * │ │ │ ▼ │ ▼ + * ▼ │ │ ╭─╮ │ │ │ + * │ │ │ │ start │ │ │ + * │ │ ┌────▼──▼─┴┐ │ ┌────▼────┐ │ + * │ ╰─────────► STOPPING │ │ │ FAILING │ │ + * │ └┬─▲─┬─────┘ │ └────┬────┘ │ + * │ abort │ │ │ │ │ + * │ ╰─╯ │ │ exit │ + * │ exit │ │ │ + * │ │ ╭─╮ │ ╰─────╮ │ ╭─╮ + * │ │ │ start │ │ │ │ start + * │ ┌────▼─▼─┴┐ ┌────▼─────┐ ┌─▼─▼─▼─┴┐ * ╰──────────────► STOPPED │ │ DETACHED │ │ FAILED │ * └┬─▲──────┘ └┬─▲───────┘ └┬─▲─────┘ * abort │ *all* │ abort │ @@ -168,10 +189,13 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand> { switch (this._state.id) { case 'unstarted': { + this._state = { + id: 'depsStarting', + started: new Deferred(), + }; + void this._startServices().then(() => { + this._onDepsStarted(); + }); + void this._anyServiceTerminated.then(() => { + this._onDepServiceExit(); + }); + return this._state.started.promise; + } + case 'failing': + case 'failed': { + return Promise.resolve({ok: false, error: [this._state.failure]}); + } + case 'initial': + case 'executingDeps': + case 'fingerprinting': + case 'depsStarting': + case 'starting': + case 'started': + case 'stopping': + case 'stopped': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + + private _onDepsStarted() { + switch (this._state.id) { + case 'depsStarting': { this._state = { id: 'starting', child: new ScriptChildProcess(this._config), - started: new Deferred(), + started: this._state.started, }; void this._state.child.started.then(() => { this._onChildStarted(); @@ -271,15 +345,69 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { this._onChildExited(); }); - return this._state.started.promise; + this._state.child.stdout.on('data', (data: string | Buffer) => { + this._logger.log({ + script: this._config, + type: 'output', + stream: 'stdout', + data, + }); + }); + this._state.child.stderr.on('data', (data: string | Buffer) => { + this._logger.log({ + script: this._config, + type: 'output', + stream: 'stderr', + data, + }); + }); + return; + } + case 'failed': { + return; } case 'initial': case 'executingDeps': case 'fingerprinting': + case 'unstarted': case 'starting': case 'started': case 'stopping': - case 'stopped': { + case 'stopped': + case 'failing': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + + private _onDepServiceExit() { + switch (this._state.id) { + case 'started': { + this._state.child.kill(); + this._state = { + id: 'failing', + failure: { + type: 'failure', + script: this._config, + // TODO(aomarks) Wrong + reason: 'service-exited-unexpectedly', + }, + }; + return; + } + case 'depsStarting': + case 'initial': + case 'executingDeps': + case 'fingerprinting': + case 'unstarted': + case 'starting': + case 'stopping': + case 'stopped': + case 'failing': + case 'failed': { throw unexpectedState(this._state); } default: { @@ -308,7 +436,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { - this._allConsumersDone(); + this._onAllConsumersDone(); }); return; } @@ -316,9 +444,12 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { + void this._anyServiceTerminated.then((result) => { if (this._state === 'after-running') { // This is expected after we're done. return; diff --git a/src/executor.ts b/src/executor.ts index b800a2dcf..ea474943f 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -19,6 +19,8 @@ import type { ServiceScriptConfig, StandardScriptConfig, } from './config.js'; +import type {Result} from './error.js'; +import type {Failure} from './event.js'; type Execution = | NoCommandScriptExecution @@ -47,7 +49,9 @@ export type FailureMode = 'no-new' | 'continue' | 'kill'; * Executes a script that has been analyzed and validated by the Analyzer. */ export class Executor { + private readonly _rootConfig: ScriptConfig; private readonly _executions = new Map(); + private readonly _allServices: Array = []; private readonly _logger: Logger; private readonly _workerPool: WorkerPool; private readonly _cache?: Cache; @@ -61,12 +65,14 @@ export class Executor { private readonly _killRunningScripts = new Deferred(); constructor( + rootConfig: ScriptConfig, logger: Logger, workerPool: WorkerPool, cache: Cache | undefined, failureMode: FailureMode, abort: Deferred ) { + this._rootConfig = rootConfig; this._logger = logger; this._workerPool = workerPool; this._cache = cache; @@ -106,6 +112,19 @@ export class Executor { }); } + /** + * Execute the root script. + */ + async execute(): Promise> { + const result = await this.getExecution(this._rootConfig).execute(); + // Wait for services to shut down. + // TODO(aomarks) In watch mode, directly-invoked scripts (and the services + // they depend on) should not block here, since they should continue + // running. + await Promise.all(this._allServices.map((service) => service.terminated)); + return result; + } + /** * Signal that a script has failed, which will potentially stop starting or * kill other scripts depending on the {@link FailureMode}. @@ -148,6 +167,7 @@ export class Executor { this._logger, this._abort.promise ); + this._allServices.push(execution); } else { execution = new StandardScriptExecution( config, diff --git a/src/test/service.test.ts b/src/test/service.test.ts index 1ed8eced2..efe1d3f7a 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -35,7 +35,7 @@ test.after.each(async (ctx) => { }); test( - 'simple consumer and service', + 'simple consumer and service with stdout', timeout(async ({rig}) => { // consumer // | @@ -69,6 +69,12 @@ test( const serviceInv = await service.nextInvocation(); await wireit.waitForLog(/Service started/); + // Confirm we show stdout/stderr from services + serviceInv.stdout('service stdout'); + await wireit.waitForLog(/service stdout/); + serviceInv.stderr('service stderr'); + await wireit.waitForLog(/service stderr/); + // The consumer starts and finishes const consumerInv = await consumer.nextInvocation(); // Wait a moment to ensure the service stays running @@ -86,4 +92,288 @@ test( }) ); +test( + 'service with standard and service deps', + timeout(async ({rig}) => { + // consumer + // | + // v + // service ---> serviceDep + // | + // v + // standardDep + + const consumer = await rig.newCommand(); + const service = await rig.newCommand(); + const standardDep = await rig.newCommand(); + const serviceDep = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + consumer: 'wireit', + service: 'wireit', + standardDep: 'wireit', + serviceDep: 'wireit', + }, + wireit: { + consumer: { + command: consumer.command, + dependencies: ['service'], + }, + service: { + command: service.command, + service: true, + dependencies: ['standardDep', 'serviceDep'], + }, + standardDep: { + command: standardDep.command, + }, + serviceDep: { + command: serviceDep.command, + service: true, + }, + }, + }, + }); + + const wireit = rig.exec('npm run consumer'); + + // The service's standard dep must finish before the service can start + const standardDepInv = await standardDep.nextInvocation(); + // Wait a moment to ensure the service hasn't started yet + await new Promise((resolve) => setTimeout(resolve, 100)); + assert.equal(service.numInvocations, 0); + assert.equal(serviceDep.numInvocations, 0); + assert.equal(consumer.numInvocations, 0); + standardDepInv.exit(0); + + // The service's own service dep must start first + const serviceDepInv = await serviceDep.nextInvocation(); + await wireit.waitForLog(/\[serviceDep\] Service started/); + + // Now the main service can start + const serviceInv = await service.nextInvocation(); + await wireit.waitForLog(/\[service\] Service started/); + + // The consumer starts and finishes + const consumerInv = await consumer.nextInvocation(); + // Wait a moment to ensure the services stay running + await new Promise((resolve) => setTimeout(resolve, 100)); + assert.ok(serviceInv.isRunning); + assert.ok(serviceDepInv.isRunning); + consumerInv.exit(0); + + // Services shut down in reverse order + await serviceInv.closed; + await wireit.waitForLog(/\[service\] Service stopped/); + await serviceDepInv.closed; + await wireit.waitForLog(/\[serviceDep\] Service stopped/); + + await wireit.exit; + assert.equal(standardDep.numInvocations, 1); + assert.equal(serviceDep.numInvocations, 1); + assert.equal(service.numInvocations, 1); + assert.equal(consumer.numInvocations, 1); + }) +); + +test( + 'standard scripts are killed when service exits unexpectedly', + timeout(async ({rig}) => { + // consumer + // | + // v + // service + + const consumer = await rig.newCommand(); + const service = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + consumer: 'wireit', + service: 'wireit', + }, + wireit: { + consumer: { + command: consumer.command, + dependencies: ['service'], + }, + service: { + command: service.command, + service: true, + }, + }, + }, + }); + + const wireit = rig.exec('npm run consumer'); + + // Service starts + const serviceInv = await service.nextInvocation(); + + // Consumer starts + const consumerInv = await consumer.nextInvocation(); + + // Service exits unexpectedly + serviceInv.exit(1); + await wireit.waitForLog(/\[service\] Service exited unexpectedly/); + + // Consumer is killed + await consumerInv.closed; + await wireit.waitForLog(/\[consumer\] Killed/); + + // Wireit exits with an error code + assert.equal((await wireit.exit).code, 1); + }) +); + +test( + 'service remembers unexpected exit failure for next start call', + timeout(async ({rig}) => { + // entrypoint + // / \ + // v v + // consumer1 consumer2 + // \ / \ + // \ / v + // v v blocker + // service + + const consumer1 = await rig.newCommand(); + const consumer2 = await rig.newCommand(); + const service = await rig.newCommand(); + const blocker = await rig.newCommand(); + + await rig.writeAtomic({ + 'package.json': { + scripts: { + entrypoint: 'wireit', + consumer1: 'wireit', + consumer2: 'wireit', + service: 'wireit', + blocker: 'wireit', + }, + wireit: { + entrypoint: { + dependencies: ['consumer1', 'consumer2'], + }, + consumer1: { + command: consumer1.command, + dependencies: ['service'], + }, + consumer2: { + command: consumer2.command, + dependencies: ['service', 'blocker'], + }, + service: { + command: service.command, + service: true, + }, + blocker: { + command: blocker.command, + }, + }, + }, + }); + + const wireit = rig.exec('npm run entrypoint', { + env: { + // Set "continue" failure mode so that consumer2 tries to start the + // service even though consumer1 will have already failed. + WIREIT_FAILURES: 'continue', + }, + }); + + // Service starts + const serviceInv = await service.nextInvocation(); + + // Blocker starts + const blockerInv = await blocker.nextInvocation(); + + // Consumer 1 starts + const consumer1Inv = await consumer1.nextInvocation(); + + // Service fails + serviceInv.exit(1); + + // Consumer 1 is killed + await consumer1Inv.closed; + + // Blocker unblocks + blockerInv.exit(0); + + // Consumer 2 can't start becuase the consumer already failed, so wireit + // exits. + assert.equal((await wireit.exit).code, 1); + }) +); + +test( + 'service shuts down when service dependency exits unexpectedly', + timeout(async ({rig}) => { + // consumer + // | + // v + // service1 + // | + // v + // service2 + + const consumer = await rig.newCommand(); + const service1 = await rig.newCommand(); + const service2 = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + consumer: 'wireit', + service1: 'wireit', + service2: 'wireit', + }, + wireit: { + consumer: { + command: consumer.command, + dependencies: ['service1'], + }, + service1: { + command: service1.command, + service: true, + dependencies: ['service2'], + }, + service2: { + command: service2.command, + service: true, + }, + }, + }, + }); + + const wireit = rig.exec('npm run consumer'); + + // Service2 starts + const service2Inv = await service2.nextInvocation(); + + // Service1 starts + const service1Inv = await service1.nextInvocation(); + + // Consumer starts + const consumerInv = await consumer.nextInvocation(); + + // Service 2 exits unexpectedly + service2Inv.exit(1); + await wireit.waitForLog(/\[service2\] Service exited unexpectedly/); + + // Consumer killed + await consumerInv.closed; + + // Service 1 shuts down + await service1Inv.closed; + + // Wireit exits with an error code + assert.equal((await wireit.exit).code, 1); + assert.equal(consumer.numInvocations, 1); + assert.equal(service1.numInvocations, 1); + assert.equal(service2.numInvocations, 1); + }) +); + test.run(); diff --git a/src/test/util/test-rig-command-child.ts b/src/test/util/test-rig-command-child.ts index bf1b9aae9..498308e91 100644 --- a/src/test/util/test-rig-command-child.ts +++ b/src/test/util/test-rig-command-child.ts @@ -59,3 +59,11 @@ if (!ipcPath) { } const socket = net.createConnection(ipcPath); new ChildIpcClient(socket); + +process.on('SIGINT', () => { + // Gracefully close the socket before we are terminated. This helps avoid + // occasional ECONNRESET errors on the other side. + socket.end(() => { + process.exit(1); + }); +}); diff --git a/src/test/util/test-rig.ts b/src/test/util/test-rig.ts index 997bb170e..5b4e7d931 100644 --- a/src/test/util/test-rig.ts +++ b/src/test/util/test-rig.ts @@ -320,34 +320,58 @@ class ExecResult { } } - private readonly _logMatchers: Array<{re: RegExp; deferred: Deferred}> = - []; + private readonly _logMatchers = new Set<{ + re: RegExp; + deferred: Deferred; + }>(); /** * Waits for the given content to be logged to either stdout or stderr. * - * When it does, it consumes all the stdout and stderr that's been emitted - * so far and returns it. + * When it does, it consumes all stdout or stderr that's been emitted up to + * that match so far. */ - async waitForLog(matcher: RegExp): Promise<{stdout: string; stderr: string}> { + waitForLog(matcher: RegExp): Promise { const deferred = new Deferred(); - this._logMatchers.push({re: matcher, deferred}); + this._logMatchers.add({re: matcher, deferred}); // In case we've already received the log we're watching for this._checkMatchersAgainstLogs(); - await deferred.promise; - const stdout = this._stdout; - const stderr = this._stderr; - this._stdout = ''; - this._stderr = ''; - return {stdout, stderr}; + return deferred.promise; } private _checkMatchersAgainstLogs() { + let stdoutLastIndex = -1; + let stderrLastIndex = -1; for (const matcher of this._logMatchers) { - if (matcher.re.test(this._stdout) || matcher.re.test(this._stderr)) { - matcher.deferred.resolve(); + const {re, deferred} = matcher; + // Use exec instead of match because otherwise if the user used the /g/ + // flag, we'll get an array and can't access the index. + const stdoutMatch = re.exec(this._stdout); + if (stdoutMatch !== null) { + deferred.resolve(); + this._logMatchers.delete(matcher); + stdoutLastIndex = Math.max( + stdoutLastIndex, + stdoutMatch.index + stdoutMatch[0].length + ); + } else { + const stderrMatch = re.exec(this._stderr); + if (stderrMatch !== null) { + deferred.resolve(); + this._logMatchers.delete(matcher); + stderrLastIndex = Math.max( + stderrLastIndex, + stderrMatch.index + stderrMatch[0].length + ); + } } } + if (stdoutLastIndex > 0) { + this._stdout = this._stdout.slice(stdoutLastIndex); + } + if (stderrLastIndex > 0) { + this._stderr = this._stderr.slice(stderrLastIndex); + } } private readonly _onStdout = (chunk: string | Buffer) => { diff --git a/src/watcher.ts b/src/watcher.ts index af18a461b..7a1fe4cb6 100644 --- a/src/watcher.ts +++ b/src/watcher.ts @@ -276,13 +276,14 @@ export class Watcher { throw unexpectedState(this._state); } const executor = new Executor( + script, this._logger, this._workerPool, this._cache, this._failureMode, this._abort ); - const result = await executor.getExecution(script).execute(); + const result = await executor.execute(); if (!result.ok) { for (const error of result.error) { this._logger.log(error); From 822a5a3f32184e19df2dd20e09fcadad21883e0e Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 28 Oct 2022 13:30:25 -0700 Subject: [PATCH 10/18] Services: Handle more starting/stopping scenarios (#486) - 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 https://github.com/google/wireit/issues/33 --- src/execution/service.ts | 152 ++++++++++-------- src/executor.ts | 9 +- src/test/service.test.ts | 169 ++++++++++++++++++++ src/test/util/test-rig-command-child.ts | 39 +++-- src/test/util/test-rig-command-interface.ts | 22 ++- src/test/util/test-rig-command.ts | 22 ++- 6 files changed, 334 insertions(+), 79 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index 9a3bcdaa4..71ccdab4e 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -17,7 +17,10 @@ import type {Failure} from '../event.js'; import type {Result} from '../error.js'; type ServiceState = - | {id: 'initial'} + | { + id: 'initial'; + entireExecutionAborted: Promise; + } | { id: 'executingDeps'; fingerprint: Deferred; @@ -90,13 +93,24 @@ function unexpectedState(state: ServiceState) { * │ │ │ * ▼ fingerprinted ▼ * │ │ │ - * │ ┌─────▼─────┐ │ - * ├─◄─ abort ───┤ UNSTARTED │ │ - * │ └─────┬─────┘ │ * │ │ │ - * │ start ╭─╮ │ - * │ │ │ start │ - * │ ┌───────▼────▼─┴┐ │ + * │ ╔══════════════════════╗ │ + * │ ║ is directly invoked? ╟── yes ──╮ │ + * │ ╚══════════╤═══════════╝ │ │ + * │ │ │ │ + * │ no │ │ + * │ │ │ │ + * │ ┌─────▼─────┐ │ │ + * ├─◄─ abort ───┤ UNSTARTED │ ▼ │ + * │ └─────┬─────┘ │ │ + * │ │ │ │ + * │ start │ │ + * │ │ │ │ + * │ │ ╭─────────◄────────╯ │ + * │ │ │ │ + * │ │ │ ╭─╮ │ + * │ │ │ │start │ + * │ ┌───────▼──▼─▼─┴┐ │ * ├─◄─ abort ─┤ DEPS_STARTING ├───── depStartErr ───►───┤ * │ └───────┬───────┘ │ * │ │ │ @@ -116,18 +130,14 @@ function unexpectedState(state: ServiceState) { * │ │ │ │ start │ │ * │ │ ┌────▼─▼─┴┐ │ │ * │ ├◄─ abort ┤ STARTED ├── exit ────────────────────┤ - * │ │ └────┬─┬─┬┘ │ │ - * │ │ │ │ │ │ │ - * │ │ │ │ ╰── depServiceExit ─►─┤ │ - * │ │ │ │ │ │ - * │ │ │ ╰───── detach ──╮ │ │ - * │ ▼ │ │ │ │ - * │ │ allConsumersDone │ │ │ - * │ │ (unless directly invoked) │ │ │ - * │ │ │ ▼ │ ▼ - * ▼ │ │ ╭─╮ │ │ │ - * │ │ │ │ start │ │ │ - * │ │ ┌────▼──▼─┴┐ │ ┌────▼────┐ │ + * │ │ └──────┬─┬┘ │ │ + * │ │ │ │ │ │ + * │ │ │ ╰── depServiceExit ─►─┤ │ + * │ │ │ │ │ + * │ │ ╰───── detach ──╮ │ │ + * │ │ │ │ │ + * ▼ │ ▼ │ ▼ + * │ │ ┌──────────┐ │ ┌────▼────┐ │ * │ ╰─────────► STOPPING │ │ │ FAILING │ │ * │ └┬─▲─┬─────┘ │ └────┬────┘ │ * │ abort │ │ │ │ │ @@ -143,7 +153,7 @@ function unexpectedState(state: ServiceState) { * ``` */ export class ServiceScriptExecution extends BaseExecutionWithCommand { - private _state: ServiceState = {id: 'initial'}; + private _state: ServiceState; private readonly _terminated = new Deferred>(); /** @@ -160,10 +170,13 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand + entireExecutionAborted: Promise ) { super(config, executor, logger); + this._state = { + id: 'initial', + entireExecutionAborted, + }; } /** @@ -173,6 +186,19 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { switch (this._state.id) { case 'initial': { + const allConsumersDone = Promise.all( + this._config.serviceConsumers.map( + (consumer) => + this._executor.getExecution(consumer).servicesNotNeeded + ) + ); + const abort = this._config.isDirectlyInvoked + ? Promise.all([this._state.entireExecutionAborted, allConsumersDone]) + : allConsumersDone; + void abort.then(() => { + this._onAbort(); + }); + this._state = { id: 'executingDeps', fingerprint: new Deferred(), @@ -220,6 +246,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand - this._executor.getExecution(consumer).servicesNotNeeded - ) - ); - void allConsumersDone.then(() => { - this._onAllConsumersDone(); - }); return; } case 'initial': @@ -458,33 +480,6 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand; /** Resolves when the first failure occurs in any script. */ private readonly _failureOccured = new Deferred(); @@ -63,6 +62,8 @@ export class Executor { private readonly _stopStartingNewScripts = new Deferred(); /** Resolves when we decide that running scripts should be killed. */ private readonly _killRunningScripts = new Deferred(); + /** Resolves when we decide that services should be stopped. */ + private readonly _stopServices = new Deferred(); constructor( rootConfig: ScriptConfig, @@ -76,7 +77,6 @@ export class Executor { this._logger = logger; this._workerPool = workerPool; this._cache = cache; - this._abort = abort; // If this entire execution is aborted because e.g. the user sent a SIGINT // to the Wireit process, then dont start new scripts, and kill running @@ -84,11 +84,14 @@ export class Executor { void abort.promise.then(() => { this._stopStartingNewScripts.resolve(); this._killRunningScripts.resolve(); + this._stopServices.resolve(); }); // If a failure occurs, then whether we stop starting new scripts or kill // running ones depends on the failure mode setting. void this._failureOccured.promise.then(() => { + // Services should stop in any mode. + this._stopServices.resolve(); switch (failureMode) { case 'continue': { break; @@ -165,7 +168,7 @@ export class Executor { config, this, this._logger, - this._abort.promise + this._stopServices.promise ); this._allServices.push(execution); } else { diff --git a/src/test/service.test.ts b/src/test/service.test.ts index efe1d3f7a..c89aba8aa 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -8,6 +8,7 @@ import {suite} from 'uvu'; import * as assert from 'uvu/assert'; import {timeout} from './util/uvu-timeout.js'; import {WireitTestRig} from './util/test-rig.js'; +import {IS_WINDOWS} from '../util/windows.js'; const test = suite<{rig: WireitTestRig}>(); @@ -376,4 +377,172 @@ test( }) ); +test( + 'directly invoked service and dependency starts and runs until SIGINT', + // service1 + // | + // v + // service2 + timeout(async ({rig}) => { + const service1 = await rig.newCommand(); + const service2 = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + service1: 'wireit', + service2: 'wireit', + }, + wireit: { + service1: { + command: service1.command, + service: true, + dependencies: ['service2'], + }, + service2: { + command: service2.command, + service: true, + }, + }, + }, + }); + + const wireit = rig.exec('npm run service1'); + + // Services start in bottom-up order. + const service2Inv = await service2.nextInvocation(); + await wireit.waitForLog(/\[service2\] Service started/); + const service1Inv = await service1.nextInvocation(); + await wireit.waitForLog(/\[service1\] Service started/); + + // Wait a moment to ensure they keep running since the user hasn't killed + // Wireit yet. + await new Promise((resolve) => setTimeout(resolve, 100)); + assert.ok(service1Inv.isRunning); + assert.ok(service2Inv.isRunning); + + // The user kills Wireit. The services stop in top-down order. + if (IS_WINDOWS) { + // We don't get graceful shutdown on Windows. + wireit.kill(); + } else { + // Wait a moment after SIGINT to ensure that until service1 actually + // exits, service2 keeps running. + const service1SigintReceived = service1Inv.interceptSigint(); + wireit.kill(); + await service1SigintReceived; + await new Promise((resolve) => setTimeout(resolve, 100)); + assert.ok(service1Inv.isRunning); + assert.ok(service2Inv.isRunning); + service1Inv.exit(0); + await wireit.waitForLog(/\[service1\] Service stopped/); + await wireit.waitForLog(/\[service2\] Service stopped/); + } + await service1Inv.closed; + assert.not(service1Inv.isRunning); + await service2Inv.closed; + assert.not(service2Inv.isRunning); + + await wireit.exit; + assert.equal(service1.numInvocations, 1); + assert.equal(service2.numInvocations, 1); + }) +); + +for (const failureMode of ['continue', 'no-new', 'kill']) { + // Even directly invoked services which don't have an error in their branch + // should stop when an error occurs elsewhere, regardless of the error mode. + // Otherwise wireit won't always exit on failures. + test( + `directly invoked service and dependency stop on error ` + + `with failure mode ${failureMode}`, + // entrypoint + // / \ + // v v + // standard service1 + // (fails) | + // v + // service2 + timeout(async ({rig}) => { + const standard = await rig.newCommand(); + const service1 = await rig.newCommand(); + const service2 = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + entrypoint: 'wireit', + standard: 'wireit', + service1: 'wireit', + service2: 'wireit', + }, + wireit: { + entrypoint: { + dependencies: ['standard', 'service1'], + }, + standard: { + command: standard.command, + }, + service1: { + command: service1.command, + service: true, + dependencies: ['service2'], + }, + service2: { + command: service2.command, + service: true, + }, + }, + }, + }); + + const wireit = rig.exec('npm run entrypoint', { + env: {WIREIT_FAILURES: failureMode}, + }); + + // Standard script starts. + const standardInv = await standard.nextInvocation(); + + // Services start in bottom-up order. + const service2Inv = await service2.nextInvocation(); + await wireit.waitForLog(/\[service2\] Service started/); + const service1Inv = await service1.nextInvocation(); + await wireit.waitForLog(/\[service1\] Service started/); + + // Wait a moment to ensure they keep running because the failure hasn't + // happened yet. + await new Promise((resolve) => setTimeout(resolve, 100)); + assert.ok(standardInv.isRunning); + assert.ok(service1Inv.isRunning); + assert.ok(service2Inv.isRunning); + + // The standard script fails. The services stop in top-down order. + if (IS_WINDOWS) { + // We don't get graceful shutdown in Windows. + standardInv.exit(1); + } else { + // Wait a moment after SIGINT to ensure that until service1 actually + // exits, service2 keeps running. + const service1SigintReceived = service1Inv.interceptSigint(); + standardInv.exit(1); + await service1SigintReceived; + await new Promise((resolve) => setTimeout(resolve, 100)); + assert.ok(service1Inv.isRunning); + assert.ok(service2Inv.isRunning); + service1Inv.exit(0); + } + + await service1Inv.closed; + assert.not(service1Inv.isRunning); + await wireit.waitForLog(/\[service1\] Service stopped/); + await service2Inv.closed; + assert.not(service2Inv.isRunning); + await wireit.waitForLog(/\[service2\] Service stopped/); + + await wireit.exit; + assert.equal(standard.numInvocations, 1); + assert.equal(service1.numInvocations, 1); + assert.equal(service2.numInvocations, 1); + }) + ); +} + test.run(); diff --git a/src/test/util/test-rig-command-child.ts b/src/test/util/test-rig-command-child.ts index 498308e91..aa672af92 100644 --- a/src/test/util/test-rig-command-child.ts +++ b/src/test/util/test-rig-command-child.ts @@ -16,10 +16,22 @@ import { } from './test-rig-command-interface.js'; class ChildIpcClient extends IpcClient { + private _sigintIntercepted = false; + + constructor(socket: net.Socket) { + super(socket); + process.on('SIGINT', () => { + // Don't exit if the rig is going to call exit manually. + if (!this._sigintIntercepted) { + this._closeSocketAndExit(0); + } + }); + } + protected override _onMessage(message: RigToChildMessage): void { switch (message.type) { case 'exit': { - process.exit(message.code); + this._closeSocketAndExit(message.code); break; } case 'stdout': { @@ -39,6 +51,13 @@ class ChildIpcClient extends IpcClient { }); break; } + case 'interceptSigint': { + this._sigintIntercepted = true; + process.on('SIGINT', () => { + this._send({type: 'sigintReceived'}); + }); + break; + } default: { console.error( `Unhandled message type ${ @@ -50,6 +69,16 @@ class ChildIpcClient extends IpcClient { } } } + + /** + * Gracefully close the socket before and exit. This helps avoid occasional + * ECONNRESET errors on the other side. + */ + private _closeSocketAndExit(code: number) { + socket.end(() => { + process.exit(code); + }); + } } const ipcPath = process.argv[2]; @@ -59,11 +88,3 @@ if (!ipcPath) { } const socket = net.createConnection(ipcPath); new ChildIpcClient(socket); - -process.on('SIGINT', () => { - // Gracefully close the socket before we are terminated. This helps avoid - // occasional ECONNRESET errors on the other side. - socket.end(() => { - process.exit(1); - }); -}); diff --git a/src/test/util/test-rig-command-interface.ts b/src/test/util/test-rig-command-interface.ts index e605a9f96..30f37edda 100644 --- a/src/test/util/test-rig-command-interface.ts +++ b/src/test/util/test-rig-command-interface.ts @@ -14,7 +14,8 @@ export type RigToChildMessage = | StdoutMessage | StderrMessage | ExitMessage - | EnvironmentRequestMessage; + | EnvironmentRequestMessage + | InterceptSigintMessage; /** * Tell the command to emit the given string to its stdout stream. @@ -40,6 +41,14 @@ export interface ExitMessage { code: number; } +/** + * The the command to wait until a SIGINT signal is received, and then send a + * messsage back instead of exiting. + */ +export interface InterceptSigintMessage { + type: 'interceptSigint'; +} + /** * Ask the command for information about its environment (argv, cwd, env). */ @@ -50,7 +59,9 @@ export interface EnvironmentRequestMessage { /** * A message sent from a spawned command to the test rig. */ -export type ChildToRigMessage = EnvironmentResponseMessage; +export type ChildToRigMessage = + | EnvironmentResponseMessage + | SigintReceivedMessage; /** * Report to the rig what cwd, argv, and environment variables were set when @@ -63,6 +74,13 @@ export interface EnvironmentResponseMessage { env: {[key: string]: string | undefined}; } +/** + * Report the rig that a SIGINT signal has been received. + */ +export interface SigintReceivedMessage { + type: 'sigintReceived'; +} + /** * Indicates the end of a JSON message on an IPC data stream. This is the * "record separator" ASCII character. diff --git a/src/test/util/test-rig-command.ts b/src/test/util/test-rig-command.ts index 764c8bbd4..9e6c55751 100644 --- a/src/test/util/test-rig-command.ts +++ b/src/test/util/test-rig-command.ts @@ -135,6 +135,7 @@ export class WireitTestRigCommandInvocation extends IpcClient< readonly command: WireitTestRigCommand; private _state: 'connected' | 'closing' | 'closed' = 'connected'; private _environmentResponse?: Deferred; + private _sigintReceived?: Deferred; constructor(socket: net.Socket, command: WireitTestRigCommand) { super(socket); @@ -164,11 +165,19 @@ export class WireitTestRigCommandInvocation extends IpcClient< this._environmentResponse.resolve(message); break; } + case 'sigintReceived': { + if (this._sigintReceived === undefined) { + throw new Error('Unexpected sigintReceived'); + } + this._sigintReceived.resolve(); + break; + } default: { throw new Error( - `Unhandled message type ${String(unreachable(message.type))}` + `Unhandled message type ${ + (unreachable(message) as ChildToRigMessage).type + }` ); - break; } } } @@ -186,6 +195,15 @@ export class WireitTestRigCommandInvocation extends IpcClient< return this._environmentResponse.promise; } + interceptSigint(): Promise { + this._assertState('connected'); + if (this._sigintReceived === undefined) { + this._sigintReceived = new Deferred(); + this._send({type: 'interceptSigint'}); + } + return this._sigintReceived.promise; + } + /** * Promise that resolves when this invocation's socket has exited, indicating * that the process has exited (or is just about to exit). From 55eccf3a20957a07c06c54452348ec04f1084c8e Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 28 Oct 2022 16:00:42 -0700 Subject: [PATCH 11/18] Services: Pass running services across watch iterations (#488) 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 https://github.com/google/wireit/issues/33 --- src/cli.ts | 3 +- src/execution/service.ts | 244 +++++++++++++++++++++++++++++++++++---- src/executor.ts | 51 ++++++-- src/test/service.test.ts | 120 +++++++++++++++++++ src/watcher.ts | 11 +- 5 files changed, 389 insertions(+), 40 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index c50304fe9..4bb8beb1c 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -99,7 +99,8 @@ const run = async (): Promise> => { workerPool, cache, options.failureMode, - abort + abort, + undefined ); const result = await executor.execute(); if (!result.ok) { diff --git a/src/execution/service.ts b/src/execution/service.ts index 71ccdab4e..b9ff95e42 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -20,28 +20,44 @@ type ServiceState = | { id: 'initial'; entireExecutionAborted: Promise; + adoptee: ServiceScriptExecution | undefined; } | { id: 'executingDeps'; - fingerprint: Deferred; + deferredFingerprint: Deferred; + adoptee: ServiceScriptExecution | undefined; } | { id: 'fingerprinting'; - fingerprint: Deferred; + deferredFingerprint: Deferred; + adoptee: ServiceScriptExecution | undefined; + } + | { + id: 'stoppingAdoptee'; + fingerprint: Fingerprint; + deferredFingerprint: Deferred; + } + | { + id: 'unstarted'; + fingerprint: Fingerprint; + adoptee: ServiceScriptExecution | undefined; } - | {id: 'unstarted'} | { id: 'depsStarting'; started: Deferred>; + fingerprint: Fingerprint; + adoptee: ServiceScriptExecution | undefined; } | { id: 'starting'; child: ScriptChildProcess; started: Deferred>; + fingerprint: Fingerprint; } | { id: 'started'; child: ScriptChildProcess; + fingerprint: Fingerprint; } | {id: 'stopping'} | {id: 'stopped'} @@ -52,7 +68,8 @@ type ServiceState = | { id: 'failed'; failure: Failure; - }; + } + | {id: 'detached'}; function unknownState(state: never) { return new Error( @@ -91,10 +108,24 @@ function unexpectedState(state: ServiceState) { * ├─◄─ abort ─┤ FINGERPRINTING │ │ * │ └───────┬────────┘ │ * │ │ │ - * ▼ fingerprinted ▼ + * │ fingerprinted ▼ * │ │ │ + * │ ╔══════════▼════════════╗ │ + * ▼ ║ adoptee has different ╟─ yes ─╮ │ + * │ ║ fingerprint? ║ │ │ + * │ ╚══════════╤════════════╝ │ │ + * │ │ ▼ │ + * │ no │ │ + * │ │ │ │ + * │ │ ┌─────────▼────────┐ │ + * ├─◄─ abort ─────────│─────◄────┤ STOPPING_ADOPTEE │ │ + * │ │ └─────────┬────────┘ │ + * │ │ │ │ + * │ ▼ adopteeStopped │ + * │ │ │ │ + * │ ├─────◄──────────────╯ │ * │ │ │ - * │ ╔══════════════════════╗ │ + * ▼ ╔══════════▼═══════════╗ │ * │ ║ is directly invoked? ╟── yes ──╮ │ * │ ╚══════════╤═══════════╝ │ │ * │ │ │ │ @@ -170,15 +201,85 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand + entireExecutionAborted: Promise, + adoptee: ServiceScriptExecution | undefined ) { super(config, executor, logger); this._state = { id: 'initial', entireExecutionAborted, + adoptee, }; } + /** + * Return the fingerprint of this service. Throws if the fingerprint is not + * yet available. Returns undefined if the service is stopped/failed/detached. + */ + get fingerprint(): Fingerprint | undefined { + switch (this._state.id) { + case 'stoppingAdoptee': + case 'unstarted': + case 'depsStarting': + case 'starting': + case 'started': { + return this._state.fingerprint; + } + case 'stopping': + case 'stopped': + case 'failed': + case 'failing': + case 'detached': { + return undefined; + } + case 'initial': + case 'executingDeps': + case 'fingerprinting': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + + detach(): ScriptChildProcess | undefined { + switch (this._state.id) { + case 'started': { + const child = this._state.child; + this._state = {id: 'detached'}; + // TODO(aomarks) There are a few promises that could still resolve even + // when we are detached, such as "abort" and "child exited". While we do + // correctly handle those events (by doing nothing in the handlers), the + // fact that the promises remain unresolved will prevent GC of old + // executions in watch mode. Those promises should probably be + // Promise.race'd to prevent that. + child.stdout.removeAllListeners(); + child.stderr.removeAllListeners(); + return child; + } + case 'stopping': + case 'stopped': + case 'failed': + case 'failing': { + return undefined; + } + case 'unstarted': + case 'depsStarting': + case 'starting': + case 'initial': + case 'executingDeps': + case 'fingerprinting': + case 'stoppingAdoptee': + case 'detached': { + throw unexpectedState(this._state); + } + default: { + throw unknownState(this._state); + } + } + } + /** * Note `execute` is a bit of a misnomer here, because we don't actually * execute the command at this stage in the case of services. @@ -201,7 +302,8 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { if (result.ok) { @@ -210,10 +312,11 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { @@ -252,12 +357,14 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { + this._onAdopteeStopped(); + }); + } + return; + } + this._state.deferredFingerprint.resolve({ + ok: true, + value: fingerprint, + }); + this._state = { + id: 'unstarted', + fingerprint, + adoptee, + }; if (this._config.isDirectlyInvoked) { void this.start(); } @@ -308,12 +445,53 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { this._onDepsStarted(); @@ -347,11 +527,13 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { this._onChildStarted(); @@ -398,12 +583,14 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand = T extends NoCommandScriptConfig ? ServiceScriptExecution : never; +export type ServiceMap = Map; + /** * What to do when a script failure occurs: * @@ -51,7 +53,9 @@ export type FailureMode = 'no-new' | 'continue' | 'kill'; export class Executor { private readonly _rootConfig: ScriptConfig; private readonly _executions = new Map(); - private readonly _allServices: Array = []; + private readonly _directlyInvokedServices: ServiceMap = new Map(); + private readonly _indirectlyInvokedServices: ServiceScriptExecution[] = []; + private readonly _previousIterationServices: ServiceMap | undefined; private readonly _logger: Logger; private readonly _workerPool: WorkerPool; private readonly _cache?: Cache; @@ -71,12 +75,14 @@ export class Executor { workerPool: WorkerPool, cache: Cache | undefined, failureMode: FailureMode, - abort: Deferred + abort: Deferred, + previousIterationServices: ServiceMap | undefined ) { this._rootConfig = rootConfig; this._logger = logger; this._workerPool = workerPool; this._cache = cache; + this._previousIterationServices = previousIterationServices; // If this entire execution is aborted because e.g. the user sent a SIGINT // to the Wireit process, then dont start new scripts, and kill running @@ -118,14 +124,30 @@ export class Executor { /** * Execute the root script. */ - async execute(): Promise> { - const result = await this.getExecution(this._rootConfig).execute(); - // Wait for services to shut down. - // TODO(aomarks) In watch mode, directly-invoked scripts (and the services - // they depend on) should not block here, since they should continue - // running. - await Promise.all(this._allServices.map((service) => service.terminated)); - return result; + async execute(): Promise> { + // TOOD(aomarks) If we have any running services from a previous watch + // iteration, we should at this point shut down any of the ones that have + // since been deleted from the build graph entirely, or which have become + // non-directly-invoked. + const errors: Failure[] = []; + const rootExecutionResult = await this.getExecution( + this._rootConfig + ).execute(); + if (!rootExecutionResult.ok) { + errors.push(...rootExecutionResult.error); + } + const indirectlyInvokedServiceResults = await Promise.all( + this._indirectlyInvokedServices.map((service) => service.terminated) + ); + for (const result of indirectlyInvokedServiceResults) { + if (!result.ok) { + errors.push(result.error); + } + } + if (errors.length > 0) { + return {ok: false, error: errors}; + } + return {ok: true, value: this._directlyInvokedServices}; } /** @@ -168,9 +190,14 @@ export class Executor { config, this, this._logger, - this._stopServices.promise + this._stopServices.promise, + this._previousIterationServices?.get(key) ); - this._allServices.push(execution); + if (config.isDirectlyInvoked) { + this._directlyInvokedServices.set(key, execution); + } else { + this._indirectlyInvokedServices.push(execution); + } } else { execution = new StandardScriptExecution( config, diff --git a/src/test/service.test.ts b/src/test/service.test.ts index c89aba8aa..e61b62e3e 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -545,4 +545,124 @@ for (const failureMode of ['continue', 'no-new', 'kill']) { ); } +test( + 'indirectly invoked service shuts down between watch iterations', + timeout(async ({rig}) => { + // consumer + // | + // v + // service + + const consumer = await rig.newCommand(); + const service = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + consumer: 'wireit', + service: 'wireit', + }, + wireit: { + consumer: { + command: consumer.command, + dependencies: ['service'], + files: ['input'], + }, + service: { + command: service.command, + service: true, + }, + }, + }, + }); + + await rig.write('input', '0'); + const wireit = rig.exec('npm run consumer --watch'); + + // Iteration 1 + { + const serviceInv = await service.nextInvocation(); + const consumerInv = await consumer.nextInvocation(); + consumerInv.exit(0); + await consumerInv.closed; + await serviceInv.closed; + } + + await rig.write('input', '1'); + + // Iteration 2 + { + const serviceInv = await service.nextInvocation(); + const consumerInv = await consumer.nextInvocation(); + consumerInv.exit(0); + await consumerInv.closed; + await serviceInv.closed; + } + + wireit.kill(); + await wireit.exit; + assert.equal(consumer.numInvocations, 2); + assert.equal(service.numInvocations, 2); + }) +); + +test( + 'directly invoked service is preserved across watch iterations', + timeout(async ({rig}) => { + // entrypoint + // / \ + // v v + // service standard + + const service = await rig.newCommand(); + const standard = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + entrypoint: 'wireit', + service: 'wireit', + standard: 'wireit', + }, + wireit: { + entrypoint: { + dependencies: ['service', 'standard'], + }, + service: { + command: service.command, + service: true, + }, + standard: { + command: standard.command, + files: ['input'], + }, + }, + }, + }); + + await rig.write('input', '0'); + const wireit = rig.exec('npm run entrypoint --watch'); + + // Iteration 1 + { + await service.nextInvocation(); + const standardInv = await standard.nextInvocation(); + standardInv.exit(0); + await standardInv.closed; + } + + await rig.write('input', '1'); + + // Iteration 2 + { + const standardInv = await standard.nextInvocation(); + standardInv.exit(0); + await standardInv.closed; + } + + wireit.kill(); + await wireit.exit; + assert.equal(service.numInvocations, 1); + assert.equal(standard.numInvocations, 2); + }) +); + test.run(); diff --git a/src/watcher.ts b/src/watcher.ts index 7a1fe4cb6..3adac1561 100644 --- a/src/watcher.ts +++ b/src/watcher.ts @@ -7,7 +7,7 @@ import chokidar from 'chokidar'; import {Analyzer} from './analyzer.js'; import {Cache} from './caching/cache.js'; -import {Executor, FailureMode} from './executor.js'; +import {Executor, FailureMode, ServiceMap} from './executor.js'; import {Logger} from './logging/logger.js'; import {Deferred} from './util/deferred.js'; import {WorkerPool} from './util/worker-pool.js'; @@ -121,6 +121,7 @@ export class Watcher { private readonly _failureMode: FailureMode; private readonly _abort: Deferred; private _debounceTimeoutId?: NodeJS.Timeout = undefined; + private _previousIterationServices?: ServiceMap = undefined; /** * The most recent analysis of the root script. As soon as we detect it might @@ -281,10 +282,14 @@ export class Watcher { this._workerPool, this._cache, this._failureMode, - this._abort + this._abort, + this._previousIterationServices ); const result = await executor.execute(); - if (!result.ok) { + if (result.ok) { + this._previousIterationServices = result.value; + } else { + this._previousIterationServices = undefined; for (const error of result.error) { this._logger.log(error); } From a3be5aa15139362d288d9a9738bdfcf6c499d9e9 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sat, 29 Oct 2022 09:21:20 -0700 Subject: [PATCH 12/18] Shut down services that have been deleted between watch iterations (#489) 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 https://github.com/google/wireit/issues/33 --- src/execution/service.ts | 9 ++++-- src/executor.ts | 49 +++++++++++++++++++++++++--- src/test/service.test.ts | 69 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 6 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index b9ff95e42..964e2db91 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -254,8 +254,13 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand> { - // TOOD(aomarks) If we have any running services from a previous watch - // iteration, we should at this point shut down any of the ones that have - // since been deleted from the build graph entirely, or which have become - // non-directly-invoked. + if ( + this._previousIterationServices !== undefined && + this._previousIterationServices.size > 0 + ) { + // If any services were removed from the graph entirely, or used to be + // directly invoked but are no longer, then stop them now. + const currentDirectlyInvokedServices = new Set(); + for (const script of findAllScripts(this._rootConfig)) { + if (script.service && script.isDirectlyInvoked) { + currentDirectlyInvokedServices.add(scriptReferenceToString(script)); + } + } + const stopPromises = []; + for (const [key, service] of this._previousIterationServices) { + if (!currentDirectlyInvokedServices.has(key)) { + const child = service.detach(); + if (child !== undefined) { + child.kill(); + stopPromises.push(child.completed); + } + this._previousIterationServices.delete(key); + } + } + await Promise.all(stopPromises); + } + const errors: Failure[] = []; const rootExecutionResult = await this.getExecution( this._rootConfig @@ -215,3 +237,22 @@ export class Executor { return execution as ConfigToExecution; } } + +/** + * Walk the dependencies of the given root script and return all scripts in the + * graph (including the root itself). + */ +function findAllScripts(root: ScriptConfig): Set { + const visited = new Set(); + const stack = [root]; + while (stack.length > 0) { + const next = stack.pop()!; + visited.add(next); + for (const dep of next.dependencies) { + if (!visited.has(dep.config)) { + stack.push(dep.config); + } + } + } + return visited; +} diff --git a/src/test/service.test.ts b/src/test/service.test.ts index e61b62e3e..fa2c1db0b 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -665,4 +665,73 @@ test( }) ); +test( + 'deleted service shuts down between watch iterations', + timeout(async ({rig}) => { + // entrypoint + // / \ + // v v + // standard service (gets deleted) + + const standard = await rig.newCommand(); + const service = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + entrypoint: 'wireit', + standard: 'wireit', + service: 'wireit', + }, + wireit: { + entrypoint: { + dependencies: ['standard', 'service'], + }, + standard: { + command: standard.command, + }, + service: { + command: service.command, + service: true, + }, + }, + }, + }); + + // Iteration 1. Both scripts start. + const wireit = rig.exec('npm run entrypoint --watch'); + const serviceInv = await service.nextInvocation(); + const standardInv1 = await standard.nextInvocation(); + standardInv1.exit(0); + await wireit.waitForLog(/Watching for file changes/); + + // Iteration 2. We update the config to delete the service. It should get + // shut down. + await rig.writeAtomic({ + 'package.json': { + scripts: { + entrypoint: 'wireit', + standard: 'wireit', + }, + wireit: { + entrypoint: { + dependencies: ['standard'], + }, + standard: { + command: standard.command, + }, + }, + }, + }); + await serviceInv.closed; + const standardInv2 = await standard.nextInvocation(); + standardInv2.exit(0); + await wireit.waitForLog(/Watching for file changes/); + + wireit.kill(); + await wireit.exit; + assert.equal(service.numInvocations, 1); + assert.equal(standard.numInvocations, 2); + }) +); + test.run(); From 6ed5b5e40222ba6ba0b1ec3085cbbaa1b7a07dbc Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sat, 29 Oct 2022 18:48:58 -0700 Subject: [PATCH 13/18] Allow service consumers to cache (#490) 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. --- src/fingerprint.ts | 8 +++-- src/test/service.test.ts | 75 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/fingerprint.ts b/src/fingerprint.ts index 89b649d99..dd12e49b8 100644 --- a/src/fingerprint.ts +++ b/src/fingerprint.ts @@ -167,9 +167,13 @@ export class Fingerprint { // because we can't know if there was an undeclared input that this script // depends on. allDependenciesAreFullyTracked && - // A no-op script. Can't produce output, so always trackable. + // A no-command script. Doesn't ever do anything itsef, so always fully + // tracked. (script.command === undefined || - // A one-shot script. Trackable if we know both its inputs and outputs. + // A service. Fully tracked if we know its inputs. Can't produce output. + (script.service && script.files !== undefined) || + // A standard script. Fully tracked if we know both its inputs and + // outputs. (script.files !== undefined && script.output !== undefined)); const fingerprint = new Fingerprint(); diff --git a/src/test/service.test.ts b/src/test/service.test.ts index fa2c1db0b..6f31d1ecd 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -734,4 +734,79 @@ test( }) ); +test( + 'service fingerprint is trackable despite never having outputs', + timeout(async ({rig}) => { + // consumer + // | + // v + // service + + const consumer = await rig.newCommand(); + const service = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + consumer: 'wireit', + service: 'wireit', + }, + wireit: { + consumer: { + command: consumer.command, + dependencies: ['service'], + files: [], + output: [], + }, + service: { + command: service.command, + service: true, + files: ['input'], + }, + }, + }, + }); + + // Run 1. Nothing cached yet. + { + await rig.write('input', '0'); + const wireit = rig.exec('npm run consumer'); + const serviceInv = await service.nextInvocation(); + const consumerInv = await consumer.nextInvocation(); + consumerInv.exit(0); + await serviceInv.closed; + await consumerInv.closed; + const {code} = await wireit.exit; + assert.equal(code, 0); + assert.equal(consumer.numInvocations, 1); + assert.equal(service.numInvocations, 1); + } + + // Run 2. No input change. Consumer output is cached, service never needs to + // start. + { + const wireit = rig.exec('npm run consumer'); + const {code} = await wireit.exit; + assert.equal(code, 0); + assert.equal(consumer.numInvocations, 1); + assert.equal(service.numInvocations, 1); + } + + // Run 3. Service input changed. That affects the service fingerprint and + // transitively affects the consumer fingerprint, so both need to run. + { + await rig.write('input', '1'); + const wireit = rig.exec('npm run consumer'); + const serviceInv = await service.nextInvocation(); + const consumerInv = await consumer.nextInvocation(); + consumerInv.exit(0); + await serviceInv.closed; + await consumerInv.closed; + const {code} = await wireit.exit; + assert.equal(code, 0); + assert.equal(consumer.numInvocations, 2); + assert.equal(service.numInvocations, 2); + } + }) +); + test.run(); From 94990ab31d004edee2efe3147b948c3ca80c06f9 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 30 Oct 2022 11:37:34 -0700 Subject: [PATCH 14/18] Services: Make services of services also be persistent (#493) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 https://github.com/google/wireit/issues/33 --- src/analyzer.ts | 12 +++++------ src/config.ts | 31 +++++++++++++++++++++++++++-- src/execution/service.ts | 20 +++++++++++-------- src/executor.ts | 28 +++++++++++++------------- src/test/analysis.test.ts | 6 +++--- src/test/service.test.ts | 42 ++++++++++++++++++++++++++------------- 6 files changed, 91 insertions(+), 48 deletions(-) diff --git a/src/analyzer.ts b/src/analyzer.ts index feae56208..add05c20d 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -946,7 +946,7 @@ export class Analyzer { private _checkForCyclesAndSortDependencies( config: LocallyValidScriptConfig | ScriptConfig | InvalidScriptConfig, trail: Set, - isDirectlyInvoked: boolean + isPersistent: boolean ): Result { if (config.state === 'valid') { // Already validated. @@ -1068,11 +1068,9 @@ export class Analyzer { this._checkForCyclesAndSortDependencies( dependency.config, trail, - // Walk through no-command scripts when determining if something is - // being directly invoked (e.g. if the top-level script has no command - // and simply delegates to one or more other scripts, then those - // dependencies are effectively being directly invoked). - isDirectlyInvoked && config.command === undefined + // Walk through no-command scripts and services when determining if + // something is persistent. + isPersistent && (config.command === undefined || config.service) ); if (!validDependencyConfigResult.ok) { return { @@ -1128,7 +1126,7 @@ export class Analyzer { // Unfortunately TypeScript doesn't narrow the ...config spread, so we // have to assign explicitly. command: config.command, - isDirectlyInvoked, + isPersistent, serviceConsumers: [], }; } else { diff --git a/src/config.ts b/src/config.ts index 2e5aa988e..0c2dd3d6b 100644 --- a/src/config.ts +++ b/src/config.ts @@ -82,9 +82,36 @@ export interface ServiceScriptConfig service: true; /** - * Whether this service is being invoked directly (e.g. `npm run serve`). + * Whether this service persists beyond the initial execution phase. + * + * When true, this service will keep running until the user exits wireit, or + * until its fingerprint changes in watch mode, requiring a restart. + * + * When false, this service will start only if it is needed by a standard + * script, and will stop when that dependent is done. We call these scripts + * "ephemeral". + * + * So, this is true when there is a path from the entrypoint script to the + * service, which does not pass through a standard script. + * + * Example: + * + * start + * (no-command) + * / \ + * ▼ ▼ + * serve:api serve:static + * (persistent service) (persistent service) + * | | + * ▼ ▼ + * serve:db build:assets + * (persistent service) (standard) + * | + * ▼ + * serve:playwright + * (ephemeral service) */ - isDirectlyInvoked: boolean; + isPersistent: boolean; /** * Scripts that depend on this service. diff --git a/src/execution/service.ts b/src/execution/service.ts index 964e2db91..b2eecfd57 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -125,9 +125,9 @@ function unexpectedState(state: ServiceState) { * │ │ │ │ * │ ├─────◄──────────────╯ │ * │ │ │ - * ▼ ╔══════════▼═══════════╗ │ - * │ ║ is directly invoked? ╟── yes ──╮ │ - * │ ╚══════════╤═══════════╝ │ │ + * ▼ ╔═══════▼════════╗ │ + * │ ║ is persistent? ╟───── yes ──╮ │ + * │ ╚═══════╤════════╝ │ │ * │ │ │ │ * │ no │ │ * │ │ │ │ @@ -298,7 +298,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { @@ -439,7 +439,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand(); - private readonly _directlyInvokedServices: ServiceMap = new Map(); - private readonly _indirectlyInvokedServices: ServiceScriptExecution[] = []; + private readonly _persistentServices: ServiceMap = new Map(); + private readonly _ephemeralServices: ServiceScriptExecution[] = []; private readonly _previousIterationServices: ServiceMap | undefined; private readonly _logger: Logger; private readonly _workerPool: WorkerPool; @@ -130,16 +130,16 @@ export class Executor { this._previousIterationServices.size > 0 ) { // If any services were removed from the graph entirely, or used to be - // directly invoked but are no longer, then stop them now. - const currentDirectlyInvokedServices = new Set(); + // persistent but are no longer, then stop them now. + const currentPersistentServices = new Set(); for (const script of findAllScripts(this._rootConfig)) { - if (script.service && script.isDirectlyInvoked) { - currentDirectlyInvokedServices.add(scriptReferenceToString(script)); + if (script.service && script.isPersistent) { + currentPersistentServices.add(scriptReferenceToString(script)); } } const stopPromises = []; for (const [key, service] of this._previousIterationServices) { - if (!currentDirectlyInvokedServices.has(key)) { + if (!currentPersistentServices.has(key)) { const child = service.detach(); if (child !== undefined) { child.kill(); @@ -158,10 +158,10 @@ export class Executor { if (!rootExecutionResult.ok) { errors.push(...rootExecutionResult.error); } - const indirectlyInvokedServiceResults = await Promise.all( - this._indirectlyInvokedServices.map((service) => service.terminated) + const ephemeralServiceResults = await Promise.all( + this._ephemeralServices.map((service) => service.terminated) ); - for (const result of indirectlyInvokedServiceResults) { + for (const result of ephemeralServiceResults) { if (!result.ok) { errors.push(result.error); } @@ -169,7 +169,7 @@ export class Executor { if (errors.length > 0) { return {ok: false, error: errors}; } - return {ok: true, value: this._directlyInvokedServices}; + return {ok: true, value: this._persistentServices}; } /** @@ -215,10 +215,10 @@ export class Executor { this._stopServices.promise, this._previousIterationServices?.get(key) ); - if (config.isDirectlyInvoked) { - this._directlyInvokedServices.set(key, execution); + if (config.isPersistent) { + this._persistentServices.set(key, execution); } else { - this._indirectlyInvokedServices.push(execution); + this._ephemeralServices.push(execution); } } else { execution = new StandardScriptExecution( diff --git a/src/test/analysis.test.ts b/src/test/analysis.test.ts index 9962177c7..9b122dbab 100644 --- a/src/test/analysis.test.ts +++ b/src/test/analysis.test.ts @@ -99,7 +99,7 @@ test('analyzes services', async ({rig}) => { } assert.equal(b.serviceConsumers.length, 1); assert.equal(b.serviceConsumers[0].name, 'd'); - assert.equal(b.isDirectlyInvoked, true); + assert.equal(b.isPersistent, true); // c const c = a.dependencies[1].config; @@ -107,7 +107,7 @@ test('analyzes services', async ({rig}) => { if (!c.service) { throw new Error('Expected service'); } - assert.equal(c.isDirectlyInvoked, true); + assert.equal(c.isPersistent, true); assert.equal(c.serviceConsumers.length, 0); assert.equal(c.services.length, 0); @@ -124,7 +124,7 @@ test('analyzes services', async ({rig}) => { if (!e.service) { throw new Error('Expected service'); } - assert.equal(e.isDirectlyInvoked, false); + assert.equal(e.isPersistent, false); assert.equal(e.serviceConsumers.length, 1); }); diff --git a/src/test/service.test.ts b/src/test/service.test.ts index 6f31d1ecd..d084ae93c 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -378,7 +378,7 @@ test( ); test( - 'directly invoked service and dependency starts and runs until SIGINT', + 'persistent service and dependency starts and runs until SIGINT', // service1 // | // v @@ -449,11 +449,11 @@ test( ); for (const failureMode of ['continue', 'no-new', 'kill']) { - // Even directly invoked services which don't have an error in their branch - // should stop when an error occurs elsewhere, regardless of the error mode. + // Even persistent services which don't have an error in their branch should + // stop when an error occurs elsewhere, regardless of the error mode. // Otherwise wireit won't always exit on failures. test( - `directly invoked service and dependency stop on error ` + + `persistent service and dependency stop on error ` + `with failure mode ${failureMode}`, // entrypoint // / \ @@ -546,7 +546,7 @@ for (const failureMode of ['continue', 'no-new', 'kill']) { } test( - 'indirectly invoked service shuts down between watch iterations', + 'ephemeral service shuts down between watch iterations', timeout(async ({rig}) => { // consumer // | @@ -606,28 +606,38 @@ test( ); test( - 'directly invoked service is preserved across watch iterations', + 'persistent services are preserved across watch iterations', timeout(async ({rig}) => { // entrypoint // / \ // v v - // service standard + // service1 standard + // | + // v + // service2 - const service = await rig.newCommand(); + const service1 = await rig.newCommand(); + const service2 = await rig.newCommand(); const standard = await rig.newCommand(); await rig.writeAtomic({ 'package.json': { scripts: { entrypoint: 'wireit', - service: 'wireit', + service1: 'wireit', + service2: 'wireit', standard: 'wireit', }, wireit: { entrypoint: { - dependencies: ['service', 'standard'], + dependencies: ['service1', 'standard'], }, - service: { - command: service.command, + service1: { + command: service1.command, + dependencies: ['service2'], + service: true, + }, + service2: { + command: service2.command, service: true, }, standard: { @@ -643,10 +653,12 @@ test( // Iteration 1 { - await service.nextInvocation(); + await service2.nextInvocation(); + await service1.nextInvocation(); const standardInv = await standard.nextInvocation(); standardInv.exit(0); await standardInv.closed; + await wireit.waitForLog(/Watching for file changes/); } await rig.write('input', '1'); @@ -656,11 +668,13 @@ test( const standardInv = await standard.nextInvocation(); standardInv.exit(0); await standardInv.closed; + await wireit.waitForLog(/Watching for file changes/); } wireit.kill(); await wireit.exit; - assert.equal(service.numInvocations, 1); + assert.equal(service1.numInvocations, 1); + assert.equal(service2.numInvocations, 1); assert.equal(standard.numInvocations, 2); }) ); From cb1e4b634565eed45b122e941e59e59b14341eb1 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Tue, 1 Nov 2022 11:24:53 -0700 Subject: [PATCH 15/18] Add missing promise resolution for graceful stop case (#497) 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 https://github.com/google/wireit/issues/33 --- src/execution/service.ts | 20 ++++++----- src/test/service.test.ts | 74 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 9 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index b2eecfd57..e8dfe9e60 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -684,11 +684,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { + // consumer + // | + // v + // service + + const consumer = await rig.newCommand(); + const service = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + consumer: 'wireit', + service: 'wireit', + }, + wireit: { + consumer: { + command: consumer.command, + dependencies: ['service'], + files: [], + output: [], + }, + service: { + command: service.command, + service: true, + files: ['input'], + }, + }, + }, + }); + + await rig.write('input', 'A'); + const wireit = rig.exec('npm run consumer --watch'); + + // 1st run with input A. Runs. + { + const serviceInv = await service.nextInvocation(); + const consumerInv1 = await consumer.nextInvocation(); + consumerInv1.exit(0); + await serviceInv.closed; + await wireit.waitForLog(/Watching for file changes/); + assert.equal(service.numInvocations, 1); + assert.equal(consumer.numInvocations, 1); + } + + // 2nd run with input B. Runs. + { + await rig.write('input', 'B'); + const serviceInv = await service.nextInvocation(); + const consumerInv1 = await consumer.nextInvocation(); + consumerInv1.exit(0); + await serviceInv.closed; + await wireit.waitForLog(/Watching for file changes/); + assert.equal(service.numInvocations, 2); + assert.equal(consumer.numInvocations, 2); + } + + // 3rd run with input A. Restored from cache. + { + await rig.write('input', 'A'); + await wireit.waitForLog(/Restored from cache/); + await wireit.waitForLog(/Watching for file changes/); + assert.equal(service.numInvocations, 2); + assert.equal(consumer.numInvocations, 2); + } + + wireit.kill(); + await wireit.exit; + assert.equal(service.numInvocations, 2); + assert.equal(consumer.numInvocations, 2); + }) +); + test.run(); From a09a0304ef147d9b8ae99e3398abe649362915cd Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Tue, 1 Nov 2022 13:45:40 -0700 Subject: [PATCH 16/18] More graceful shutdown of stale services (#500) 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 https://github.com/google/wireit/issues/33 --- src/execution/service.ts | 45 ++++++++++++++++++++++------------------ src/executor.ts | 10 +++------ src/test/service.test.ts | 13 ++++++++++++ 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index e8dfe9e60..6085c98b6 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -243,6 +243,10 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { - this._onAbort(); + void this.abort(); }); this._state = { @@ -414,20 +418,16 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { - this._onAdopteeStopped(); - }); - } + // There is a previous running version of this service, but the + // fingerprint changed, so we need to restart it. + this._state = { + id: 'stoppingAdoptee', + fingerprint, + deferredFingerprint: this._state.deferredFingerprint, + }; + void adoptee.abort().then(() => { + this._onAdopteeStopped(); + }); return; } this._state.deferredFingerprint.resolve({ @@ -724,16 +724,20 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { switch (this._state.id) { case 'started': { this._state.child.kill(); this._state = {id: 'stopping'}; - return; + break; } case 'starting': { this._state = {id: 'stopping'}; - return; + break; } case 'initial': case 'executingDeps': @@ -742,19 +746,20 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand undefined); } private _enterStoppedState() { diff --git a/src/executor.ts b/src/executor.ts index 7cf53feec..8c1cfba63 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -137,18 +137,14 @@ export class Executor { currentPersistentServices.add(scriptReferenceToString(script)); } } - const stopPromises = []; + const abortPromises = []; for (const [key, service] of this._previousIterationServices) { if (!currentPersistentServices.has(key)) { - const child = service.detach(); - if (child !== undefined) { - child.kill(); - stopPromises.push(child.completed); - } + abortPromises.push(service.abort()); this._previousIterationServices.delete(key); } } - await Promise.all(stopPromises); + await Promise.all(abortPromises); } const errors: Failure[] = []; diff --git a/src/test/service.test.ts b/src/test/service.test.ts index ce381f8e3..338fe6a74 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -720,6 +720,7 @@ test( // Iteration 2. We update the config to delete the service. It should get // shut down. + const serviceSigint = IS_WINDOWS ? undefined : serviceInv.interceptSigint(); await rig.writeAtomic({ 'package.json': { scripts: { @@ -736,6 +737,18 @@ test( }, }, }); + if (!IS_WINDOWS) { + // Ensure that we continue to forward stdout/stderr while a stale service + // is being stopped. This won't be the case if we naively detach from the + // first execution, since then we'd stop listening for the output event + // listeners. Note we don't get graceful shutdown in Windows, so just skip + // this in Windows. + await serviceSigint; + serviceInv.stdout('Service shutting down'); + await wireit.waitForLog(/Service shutting down/); + serviceInv.stdout('Service shutting down'); + serviceInv.exit(0); + } await serviceInv.closed; const standardInv2 = await standard.nextInvocation(); standardInv2.exit(0); From f593a513c9e30f80c530bfba2ad3065bc7dc23f9 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Thu, 3 Nov 2022 18:22:41 -0700 Subject: [PATCH 17/18] Services: Fix memory leaks in watch mode (#504) 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 https://github.com/google/wireit/issues/33 --- .github/workflows/tests.yml | 14 ++ package.json | 9 + src/cli.ts | 19 +-- src/event.ts | 11 +- src/execution/base.ts | 15 ++ src/execution/service.ts | 19 ++- src/executor.ts | 68 ++++++-- src/logging/default-logger.ts | 6 + src/test/gc.test.ts | 313 ++++++++++++++++++++++++++++++++++ src/watcher.ts | 49 ++---- tsconfig.json | 2 +- 11 files changed, 464 insertions(+), 61 deletions(-) create mode 100644 src/test/gc.test.ts diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 9ade98657..65e578c1e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -64,3 +64,17 @@ jobs: - run: npm ci - run: npm run lint - run: npm run format:check + + test-garbage-collection: + timeout-minutes: 5 + runs-on: ubuntu-20.04 + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-node@v3 + with: + node-version: 16 + cache: npm + - uses: google/wireit@setup-github-actions-caching/v1 + + - run: npm ci + - run: npm run test:gc diff --git a/package.json b/package.json index 0314587c3..ff96ad82a 100644 --- a/package.json +++ b/package.json @@ -44,6 +44,7 @@ "test:failures": "wireit", "test:freshness": "wireit", "test:ide": "wireit", + "test:gc": "wireit", "test:glob": "wireit", "test:json-schema": "wireit", "test:optimize-mkdirs": "wireit", @@ -221,6 +222,14 @@ "files": [], "output": [] }, + "test:gc": { + "command": "cross-env NODE_OPTIONS=--enable-source-maps node --expose-gc node_modules/uvu/bin.js lib/test \"^gc\\.test\\.js$\"", + "dependencies": [ + "build" + ], + "files": [], + "output": [] + }, "test:glob": { "command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^glob\\.test\\.js$\"", "dependencies": [ diff --git a/src/cli.ts b/src/cli.ts index 4bb8beb1c..737f256e5 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -9,7 +9,6 @@ import {Analyzer} from './analyzer.js'; import {Executor} from './executor.js'; import {WorkerPool} from './util/worker-pool.js'; import {unreachable} from './util/unreachable.js'; -import {Deferred} from './util/deferred.js'; import {Failure} from './event.js'; import {logger, getOptions} from './cli-options.js'; @@ -71,22 +70,20 @@ const run = async (): Promise> => { } } - const abort = new Deferred(); - process.on('SIGINT', () => { - abort.resolve(); - }); - if (options.watch) { const {Watcher} = await import('./watcher.js'); - await Watcher.watch( + const watcher = new Watcher( options.script, options.extraArgs, logger, workerPool, cache, - options.failureMode, - abort + options.failureMode ); + process.on('SIGINT', () => { + watcher.abort(); + }); + await watcher.watch(); } else { const analyzer = new Analyzer(); const {config} = await analyzer.analyze(options.script, options.extraArgs); @@ -99,9 +96,11 @@ const run = async (): Promise> => { workerPool, cache, options.failureMode, - abort, undefined ); + process.on('SIGINT', () => { + executor.abort(); + }); const result = await executor.execute(); if (!result.ok) { return result; diff --git a/src/event.ts b/src/event.ts index 60963fbbc..9b3a147c8 100644 --- a/src/event.ts +++ b/src/event.ts @@ -93,7 +93,8 @@ export type Failure = | DependencyOnMissingPackageJson | DependencyOnMissingScript | DependencyInvalid - | ServiceExitedUnexpectedly; + | ServiceExitedUnexpectedly + | Aborted; interface ErrorBase extends EventBase { @@ -249,6 +250,14 @@ export interface ServiceExitedUnexpectedly extends ErrorBase { reason: 'service-exited-unexpectedly'; } +/** + * A script was killed or is refusing to run because it was intentionally + * aborted. Usually due to an error occuring in another script somewhere. + */ +export interface Aborted extends ErrorBase { + reason: 'aborted'; +} + /** * We reached the point of doing cyclic dependency checking, and one of our * transitive dependencies had not transitioned to being locally validated. diff --git a/src/execution/base.ts b/src/execution/base.ts index dfdd6a89e..28974c537 100644 --- a/src/execution/base.ts +++ b/src/execution/base.ts @@ -26,6 +26,20 @@ export type ExecutionResult = Result; */ export type FailureMode = 'no-new' | 'continue' | 'kill'; +let executionConstructorHook: + | ((executor: BaseExecution) => void) + | undefined; + +/** + * For GC testing only. A function that is called whenever an Execution is + * constructed. + */ +export function registerExecutionConstructorHook( + fn: typeof executionConstructorHook +) { + executionConstructorHook = fn; +} + /** * A single execution of a specific script. */ @@ -36,6 +50,7 @@ export abstract class BaseExecution { private _fingerprint?: Promise; constructor(config: T, executor: Executor, logger: Logger) { + executionConstructorHook?.(this); this._config = config; this._executor = executor; this._logger = logger; diff --git a/src/execution/service.ts b/src/execution/service.ts index 6085c98b6..0ffe2dc13 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -525,6 +525,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand; */ export type FailureMode = 'no-new' | 'continue' | 'kill'; +let executorConstructorHook: ((executor: Executor) => void) | undefined; + +/** + * For GC testing only. A function that is called whenever an Executor is + * constructed. + */ +export function registerExecutorConstructorHook( + fn: typeof executorConstructorHook +) { + executorConstructorHook = fn; +} + /** * Executes a script that has been analyzed and validated by the Analyzer. */ @@ -75,24 +87,15 @@ export class Executor { workerPool: WorkerPool, cache: Cache | undefined, failureMode: FailureMode, - abort: Deferred, previousIterationServices: ServiceMap | undefined ) { + executorConstructorHook?.(this); this._rootConfig = rootConfig; this._logger = logger; this._workerPool = workerPool; this._cache = cache; this._previousIterationServices = previousIterationServices; - // If this entire execution is aborted because e.g. the user sent a SIGINT - // to the Wireit process, then dont start new scripts, and kill running - // ones. - void abort.promise.then(() => { - this._stopStartingNewScripts.resolve(); - this._killRunningScripts.resolve(); - this._stopServices.resolve(); - }); - // If a failure occurs, then whether we stop starting new scripts or kill // running ones depends on the failure mode setting. void this._failureOccured.promise.then(() => { @@ -121,6 +124,16 @@ export class Executor { }); } + /** + * If this entire execution is aborted because e.g. the user sent a SIGINT to + * the Wireit process, then dont start new scripts, and kill running ones. + */ + abort() { + this._stopStartingNewScripts.resolve(); + this._killRunningScripts.resolve(); + this._stopServices.resolve(); + } + /** * Execute the root script. */ @@ -154,6 +167,17 @@ export class Executor { if (!rootExecutionResult.ok) { errors.push(...rootExecutionResult.error); } + // Wait for all persistent services to start. + for (const service of this._persistentServices.values()) { + // Persistent services start automatically, so calling start() here should + // be a no-op, but it lets us get the started promise. + const result = await service.start(); + if (!result.ok) { + errors.push(...result.error); + } + } + // Wait for all ephemeral services to have terminated (either started and + // stopped, or never needed to start). const ephemeralServiceResults = await Promise.all( this._ephemeralServices.map((service) => service.terminated) ); @@ -204,12 +228,34 @@ export class Executor { if (config.command === undefined) { execution = new NoCommandScriptExecution(config, this, this._logger); } else if (config.service) { + const adoptee = this._previousIterationServices?.get(key); + if (adoptee !== undefined) { + // Remove the adoptee from the map so that this executor doesn't hold + // a reference to it. Otherwise, we'll maintain a chain of references + // going all the way back through all previous executions, which will + // leak memory in watch mode. + // + // executor N + // break this -----> | [previousIterationServices] + // reference v + // service N-1 + // | [executor] + // v + // executor N-1 + // | [previousIterationServices] + // v + // sevice N-2 + // | [executor] + // v + // ... + this._previousIterationServices!.delete(key); + } execution = new ServiceScriptExecution( config, this, this._logger, this._stopServices.promise, - this._previousIterationServices?.get(key) + adoptee ); if (config.isPersistent) { this._persistentServices.set(key, execution); diff --git a/src/logging/default-logger.ts b/src/logging/default-logger.ts index ce08a5cfc..db6ae7379 100644 --- a/src/logging/default-logger.ts +++ b/src/logging/default-logger.ts @@ -204,6 +204,12 @@ export class DefaultLogger implements Logger { console.error(`❌${prefix} Service exited unexpectedly`); break; } + case 'aborted': { + // This event isn't very useful to log. Things get aborted only + // because of a failure somewhere else, which should already get + // reported. + break; + } } break; } diff --git a/src/test/gc.test.ts b/src/test/gc.test.ts new file mode 100644 index 000000000..00f6133a9 --- /dev/null +++ b/src/test/gc.test.ts @@ -0,0 +1,313 @@ +/** + * @license + * Copyright 2022 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import {suite} from 'uvu'; +import * as assert from 'uvu/assert'; +import {timeout} from './util/uvu-timeout.js'; +import {WireitTestRig} from './util/test-rig.js'; +import { + Executor, + registerExecutorConstructorHook, + ServiceMap, +} from '../executor.js'; +import {Analyzer} from '../analyzer.js'; +import {DefaultLogger} from '../logging/default-logger.js'; +import {WorkerPool} from '../util/worker-pool.js'; +import {registerExecutionConstructorHook} from '../execution/base.js'; + +const test = suite<{rig: WireitTestRig}>(); + +let numLiveExecutors = 0; +let numLiveExecutions = 0; + +test.before.each(async (ctx) => { + try { + const executorFinalizationRegistry = new FinalizationRegistry(() => { + numLiveExecutors--; + }); + registerExecutorConstructorHook((executor) => { + numLiveExecutors++; + executorFinalizationRegistry.register(executor, null); + }); + + const executionFinalizationRegistry = new FinalizationRegistry(() => { + numLiveExecutions--; + }); + registerExecutionConstructorHook((execution) => { + numLiveExecutions++; + executionFinalizationRegistry.register(execution, null); + }); + ctx.rig = new WireitTestRig(); + await ctx.rig.setup(); + } catch (error) { + // Uvu has a bug where it silently ignores failures in before and after, + // see https://github.com/lukeed/uvu/issues/191. + console.error('uvu before error', error); + process.exit(1); + } +}); + +test.after.each(async (ctx) => { + try { + numLiveExecutors = 0; + numLiveExecutions = 0; + await ctx.rig.cleanup(); + } catch (error) { + // Uvu has a bug where it silently ignores failures in before and after, + // see https://github.com/lukeed/uvu/issues/191. + console.error('uvu after error', error); + process.exit(1); + } +}); + +async function retryWithGcUntilCallbackDoesNotThrow( + cb: () => void +): Promise { + for (const wait of [0, 10, 100, 500, 1000]) { + global.gc(); + try { + cb(); + return; + } catch { + // Ignore + } + await new Promise((resolve) => setTimeout(resolve, wait)); + } + // Final attempt without a try, to let the exception bubble up. + cb(); +} + +test( + 'standard garbage collection', + timeout(async ({rig}) => { + const standard = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + standard: 'wireit', + }, + wireit: { + standard: { + command: standard.command, + }, + }, + }, + }); + + const logger = new DefaultLogger(rig.temp); + const script = await new Analyzer().analyze( + {packageDir: rig.temp, name: 'standard'}, + [] + ); + if (!script.config.ok) { + for (const error of script.config.error) { + logger.log(error); + } + throw new Error(`Analysis error`); + } + + const workerPool = new WorkerPool(Infinity); + + const numIterations = 10; + for (let i = 0; i < numIterations; i++) { + const executor = new Executor( + script.config.value, + logger, + workerPool, + undefined, + 'no-new', + undefined + ); + const resultPromise = executor.execute(); + assert.ok(numLiveExecutors >= 1); + assert.ok(numLiveExecutions >= 1); + (await standard.nextInvocation()).exit(0); + const result = await resultPromise; + if (!result.ok) { + for (const error of result.error) { + logger.log(error); + } + throw new Error(`Execution error`); + } + } + + await retryWithGcUntilCallbackDoesNotThrow(() => { + // TODO(aomarks) Not sure why it's 1 instead of 0, but as long as it's not + // numIterations we're OK. + assert.equal(numLiveExecutors, 1); + assert.equal(numLiveExecutions, 1); + }); + assert.equal(standard.numInvocations, numIterations); + }) +); + +test( + 'persistent service garbage collection', + timeout(async ({rig}) => { + const service = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + service: 'wireit', + }, + wireit: { + service: { + command: service.command, + service: true, + }, + }, + }, + }); + + const logger = new DefaultLogger(rig.temp); + const script = await new Analyzer().analyze( + {packageDir: rig.temp, name: 'service'}, + [] + ); + if (!script.config.ok) { + for (const error of script.config.error) { + logger.log(error); + } + throw new Error(`Analysis error`); + } + + const workerPool = new WorkerPool(Infinity); + + const numIterations = 10; + let previousServices: ServiceMap | undefined; + for (let i = 0; i < numIterations; i++) { + const executor = new Executor( + script.config.value, + logger, + workerPool, + undefined, + 'no-new', + previousServices + ); + const resultPromise = executor.execute(); + assert.ok(numLiveExecutors >= 1); + assert.ok(numLiveExecutions >= 1); + const result = await resultPromise; + if (!result.ok) { + for (const error of result.error) { + logger.log(error); + } + throw new Error(`Execution error`); + } + previousServices = result.value; + if (i === 0) { + await service.nextInvocation(); + } + } + + for (const service of previousServices!.values()) { + await service.abort(); + } + + await retryWithGcUntilCallbackDoesNotThrow(() => { + // TODO(aomarks) Not sure why it's 1 instead of 0, but as long as it's not + // numIterations we're OK. + assert.equal(numLiveExecutors, 1); + assert.equal(numLiveExecutions, 1); + }); + assert.equal(service.numInvocations, 1); + }) +); + +test( + 'no-command, standard, persistent service, and ephemeral service garbage collection', + timeout(async ({rig}) => { + const standard = await rig.newCommand(); + const servicePersistent = await rig.newCommand(); + const serviceEphemeral = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + entrypoint: 'wireit', + standard: 'wireit', + servicePersistent: 'wireit', + serviceEphemeral: 'wireit', + }, + wireit: { + entrypoint: { + dependencies: ['standard', 'servicePersistent'], + }, + standard: { + command: standard.command, + dependencies: ['serviceEphemeral'], + }, + servicePersistent: { + command: servicePersistent.command, + service: true, + }, + serviceEphemeral: { + command: serviceEphemeral.command, + service: true, + }, + }, + }, + }); + + const logger = new DefaultLogger(rig.temp); + const script = await new Analyzer().analyze( + {packageDir: rig.temp, name: 'entrypoint'}, + [] + ); + if (!script.config.ok) { + for (const error of script.config.error) { + logger.log(error); + } + throw new Error(`Analysis error`); + } + + const workerPool = new WorkerPool(Infinity); + + const numIterations = 10; + let previousServices: ServiceMap | undefined; + for (let i = 0; i < numIterations; i++) { + const executor = new Executor( + script.config.value, + logger, + workerPool, + undefined, + 'no-new', + previousServices + ); + const resultPromise = executor.execute(); + assert.ok(numLiveExecutors >= 1); + assert.ok(numLiveExecutions >= 1); + if (i === 0) { + await servicePersistent.nextInvocation(); + } + await serviceEphemeral.nextInvocation(); + (await standard.nextInvocation()).exit(0); + const result = await resultPromise; + if (!result.ok) { + for (const error of result.error) { + logger.log(error); + } + throw new Error(`Execution error`); + } + previousServices = result.value; + } + + for (const service of previousServices!.values()) { + await service.abort(); + } + + await retryWithGcUntilCallbackDoesNotThrow(() => { + // TODO(aomarks) Not sure why it's 1 and 4 instead of 0, but as long as + // it's not a factor of numIterations we're OK. + assert.equal(numLiveExecutors, 1); + assert.equal(numLiveExecutions, 4); + }); + assert.equal(standard.numInvocations, numIterations); + assert.equal(servicePersistent.numInvocations, 1); + assert.equal(serviceEphemeral.numInvocations, numIterations); + }) +); + +test.run(); diff --git a/src/watcher.ts b/src/watcher.ts index 3adac1561..ca874cf1f 100644 --- a/src/watcher.ts +++ b/src/watcher.ts @@ -85,31 +85,6 @@ const DEBOUNCE_MS = 0; * when they change. */ export class Watcher { - static async watch( - rootScript: ScriptReference, - extraArgs: string[] | undefined, - logger: Logger, - workerPool: WorkerPool, - cache: Cache | undefined, - failureMode: FailureMode, - abort: Deferred - ): Promise { - const watcher = new Watcher( - rootScript, - extraArgs, - logger, - workerPool, - cache, - failureMode, - abort - ); - void watcher._startRun(); - void abort.promise.then(() => { - watcher._onAbort(); - }); - return watcher._finished.promise; - } - /** See {@link WatcherState} */ private _state: WatcherState = 'initial'; @@ -119,7 +94,7 @@ export class Watcher { private readonly _workerPool: WorkerPool; private readonly _cache?: Cache; private readonly _failureMode: FailureMode; - private readonly _abort: Deferred; + private _executor?: Executor; private _debounceTimeoutId?: NodeJS.Timeout = undefined; private _previousIterationServices?: ServiceMap = undefined; @@ -148,14 +123,13 @@ export class Watcher { */ private readonly _finished = new Deferred(); - private constructor( + constructor( rootScript: ScriptReference, extraArgs: string[] | undefined, logger: Logger, workerPool: WorkerPool, cache: Cache | undefined, - failureMode: FailureMode, - abort: Deferred + failureMode: FailureMode ) { this._rootScript = rootScript; this._extraArgs = extraArgs; @@ -163,7 +137,11 @@ export class Watcher { this._workerPool = workerPool; this._failureMode = failureMode; this._cache = cache; - this._abort = abort; + } + + watch(): Promise { + void this._startRun(); + return this._finished.promise; } private _startDebounce(): void { @@ -276,16 +254,15 @@ export class Watcher { if (this._state !== 'running') { throw unexpectedState(this._state); } - const executor = new Executor( + this._executor = new Executor( script, this._logger, this._workerPool, this._cache, this._failureMode, - this._abort, this._previousIterationServices ); - const result = await executor.execute(); + const result = await this._executor.execute(); if (result.ok) { this._previousIterationServices = result.value; } else { @@ -409,7 +386,11 @@ export class Watcher { } } - private _onAbort(): void { + abort(): void { + if (this._executor !== undefined) { + this._executor.abort(); + this._executor = undefined; + } switch (this._state) { case 'debouncing': case 'watching': { diff --git a/tsconfig.json b/tsconfig.json index b3b5b8a87..4174fc883 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -5,7 +5,7 @@ "moduleResolution": "node", "esModuleInterop": true, "useDefineForClassFields": false, - "lib": ["es2020"], + "lib": ["es2022"], "rootDir": "src", "outDir": "lib", "strict": true, From e30f98a216a47dd30a9e3a9c4c7cac2c48d88100 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sat, 5 Nov 2022 10:38:37 -0700 Subject: [PATCH 18/18] Services: Misc fixes mostly related to error handling (#507) Kind of a grab bag of fixes mostly related to error handling. Part of https://github.com/google/wireit/issues/33 --- CHANGELOG.md | 6 + src/cli.ts | 16 +++ src/event.ts | 9 ++ src/execution/base.ts | 2 +- src/execution/service.ts | 206 ++++++++++++++++++++++++---------- src/execution/standard.ts | 23 +--- src/executor.ts | 38 +++---- src/logging/default-logger.ts | 8 +- src/test/service.test.ts | 58 +++++++++- src/test/util/test-rig.ts | 24 ++-- 10 files changed, 271 insertions(+), 119 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f8856cd4..aeba85eba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,11 +8,17 @@ Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added + - Added `"service": true` setting, which is well suited for long-running processes like servers. A service is started either when it is invoked directly, or when another script that depends on it is ready to run. A service is stopped when all scripts that depend on it have finished, or when Wireit is exited. +### Fixed + +- Fixed memory leak in watch mode. + ## [0.7.2] - 2022-09-25 ### Fixed diff --git a/src/cli.ts b/src/cli.ts index 737f256e5..6b38d3727 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -105,6 +105,22 @@ const run = async (): Promise> => { if (!result.ok) { return result; } + const persistentServices = result.value; + if (persistentServices.size > 0) { + const failures: Failure[] = []; + for (const service of persistentServices.values()) { + const result = await service.terminated; + if (!result.ok) { + failures.push(result.error); + } + } + if (failures.length > 0) { + return { + ok: false, + error: failures, + }; + } + } } return {ok: true, value: undefined}; }; diff --git a/src/event.ts b/src/event.ts index 9b3a147c8..de5c3047b 100644 --- a/src/event.ts +++ b/src/event.ts @@ -94,6 +94,7 @@ export type Failure = | DependencyOnMissingScript | DependencyInvalid | ServiceExitedUnexpectedly + | DependencyServiceExitedUnexpectedly | Aborted; interface ErrorBase @@ -250,6 +251,14 @@ export interface ServiceExitedUnexpectedly extends ErrorBase { reason: 'service-exited-unexpectedly'; } +/** + * A service that we depend on exited before it was supposed to, causing us to + * fail as well. + */ +export interface DependencyServiceExitedUnexpectedly extends ErrorBase { + reason: 'dependency-service-exited-unexpectedly'; +} + /** * A script was killed or is refusing to run because it was intentionally * aborted. Usually due to an error occuring in another script somewhere. diff --git a/src/execution/base.ts b/src/execution/base.ts index 28974c537..ed0864fcc 100644 --- a/src/execution/base.ts +++ b/src/execution/base.ts @@ -141,7 +141,7 @@ export abstract class BaseExecutionWithCommand< const errors: Failure[] = []; for (const result of results) { if (!result.ok) { - errors.push(...result.error); + errors.push(result.error); } } if (errors.length > 0) { diff --git a/src/execution/service.ts b/src/execution/service.ts index 0ffe2dc13..952b8870e 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -44,14 +44,14 @@ type ServiceState = } | { id: 'depsStarting'; - started: Deferred>; + started: Deferred>; fingerprint: Fingerprint; adoptee: ServiceScriptExecution | undefined; } | { id: 'starting'; child: ScriptChildProcess; - started: Deferred>; + started: Deferred>; fingerprint: Fingerprint; } | { @@ -59,10 +59,14 @@ type ServiceState = child: ScriptChildProcess; fingerprint: Fingerprint; } - | {id: 'stopping'} + | { + id: 'stopping'; + child: ScriptChildProcess; + } | {id: 'stopped'} | { id: 'failing'; + child: ScriptChildProcess; failure: Failure; } | { @@ -146,20 +150,27 @@ function unexpectedState(state: ServiceState) { * │ └───────┬───────┘ │ * │ │ │ * │ depsStarted ▼ - * │ │ ╭─╮ │ - * │ │ │ start │ - * │ ┌────▼──▼─┴┐ │ + * │ │ │ + * │ │ │ + * ▼ ╔══════▼═══════╗ │ + * │ ║ has adoptee? ╟───── yes ───╮ │ + * │ ╚══════╤═══════╝ │ │ + * │ │ │ │ + * │ no │ │ + * │ │ ╭─╮ ▼ │ + * │ │ │ start │ │ + * │ ┌────▼──▼─┴┐ │ │ * │ ╭◄─ abort ┤ STARTING ├──── startErr ──────►──────┤ - * │ │ └────┬────┬┘ │ - * │ │ │ │ │ + * │ │ └────┬────┬┘ │ │ + * │ │ │ │ │ │ * │ │ │ ╰─ depServiceExit ─►─╮ │ - * ▼ │ │ │ │ - * │ │ │ │ │ - * │ ▼ │ ▼ ▼ - * │ │ started │ │ - * │ │ │ ╭─╮ │ │ - * │ │ │ │ start │ │ - * │ │ ┌────▼─▼─┴┐ │ │ + * ▼ │ │ │ │ │ + * │ │ │ │ │ │ + * │ ▼ │ ▼ ▼ ▼ + * │ │ started │ │ │ + * │ │ ╭─╮ │ ╭─────────◄────────╯ │ │ + * │ │ start │ │ │ │ │ + * │ │ ┌▼─┴─▼──▼─┐ │ │ * │ ├◄─ abort ┤ STARTED ├── exit ────────────────────┤ * │ │ └──────┬─┬┘ │ │ * │ │ │ │ │ │ @@ -252,13 +263,6 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand> { + start(): Promise> { switch (this._state.id) { case 'unstarted': { + const started = new Deferred>(); this._state = { id: 'depsStarting', - started: new Deferred(), + started, fingerprint: this._state.fingerprint, adoptee: this._state.adoptee, }; - void this._startServices().then(() => { - this._onDepsStarted(); + void this._startServices().then((result) => { + if (result.ok) { + this._onDepsStarted(); + } else { + this._onDepStartErr(result); + } }); - void this._anyServiceTerminated.then(() => { - this._onDepServiceExit(); + void this.terminated.then((result) => { + if (started.settled) { + return; + } + // This service terminated before it started. Either a failure occured + // or we were aborted. If we were aborted, convert to a failure, + // because this is the start method, where ok means the service + // started. + started.resolve( + !result.ok + ? result + : { + ok: false, + error: { + type: 'failure', + script: this._config, + reason: 'aborted', + }, + } + ); }); return this._state.started.promise; } @@ -534,19 +562,17 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { - this._onChildStarted(); - }); + let child = this._state.adoptee?.detach(); + if (child === undefined) { + child = new ScriptChildProcess(this._config); + this._state = { + id: 'starting', + child, + started: this._state.started, + fingerprint: this._state.fingerprint, + }; + void this._state.child.started.then(() => { + this._onChildStarted(); + }); + } else { + this._state.started.resolve({ok: true, value: undefined}); + this._state = { + id: 'started', + child, + fingerprint: this._state.fingerprint, + }; + } void this._state.child.completed.then(() => { this._onChildExited(); }); @@ -595,6 +630,9 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { + this._onDepServiceExit(); + }); return; } case 'failed': { @@ -619,22 +657,68 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { + void this._anyServiceTerminated.then(() => { if (this._state === 'after-running') { // This is expected after we're done. return; } - if (result.ok) { - // This should never happen and indicates an internal error. The - // service believed that nothing was depending on it anymore, but - // we're still running. - earlyServiceTermination = { - script: this._config, - type: 'failure', - reason: 'unknown-error-thrown', - error: new Error( - 'Internal error: service dependency terminated unexpectedly' - ), - }; - } else { - // The service knows it exited too early. Propagate that error. - earlyServiceTermination = result.error; - } + earlyServiceTermination = { + script: this._config, + type: 'failure', + reason: 'dependency-service-exited-unexpectedly', + }; // Stop running. If a service we depend on is down, then we know we're // in an invalid state too. child.kill(); diff --git a/src/executor.ts b/src/executor.ts index c4e47f16a..dc527dfb5 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -67,7 +67,7 @@ export class Executor { private readonly _executions = new Map(); private readonly _persistentServices: ServiceMap = new Map(); private readonly _ephemeralServices: ServiceScriptExecution[] = []; - private readonly _previousIterationServices: ServiceMap | undefined; + private _previousIterationServices: ServiceMap | undefined; private readonly _logger: Logger; private readonly _workerPool: WorkerPool; private readonly _cache?: Cache; @@ -132,6 +132,11 @@ export class Executor { this._stopStartingNewScripts.resolve(); this._killRunningScripts.resolve(); this._stopServices.resolve(); + if (this._previousIterationServices !== undefined) { + for (const service of this._previousIterationServices.values()) { + void service.abort(); + } + } } /** @@ -173,7 +178,7 @@ export class Executor { // be a no-op, but it lets us get the started promise. const result = await service.start(); if (!result.ok) { - errors.push(...result.error); + errors.push(result.error); } } // Wait for all ephemeral services to have terminated (either started and @@ -186,6 +191,11 @@ export class Executor { errors.push(result.error); } } + // All previous services are either now adopted or stopped. Remove the + // reference to this map to allow for garbage collection, otherwise in watch + // mode we'll have a chain of references all the way back through every + // iteration. + this._previousIterationServices = undefined; if (errors.length > 0) { return {ok: false, error: errors}; } @@ -228,34 +238,12 @@ export class Executor { if (config.command === undefined) { execution = new NoCommandScriptExecution(config, this, this._logger); } else if (config.service) { - const adoptee = this._previousIterationServices?.get(key); - if (adoptee !== undefined) { - // Remove the adoptee from the map so that this executor doesn't hold - // a reference to it. Otherwise, we'll maintain a chain of references - // going all the way back through all previous executions, which will - // leak memory in watch mode. - // - // executor N - // break this -----> | [previousIterationServices] - // reference v - // service N-1 - // | [executor] - // v - // executor N-1 - // | [previousIterationServices] - // v - // sevice N-2 - // | [executor] - // v - // ... - this._previousIterationServices!.delete(key); - } execution = new ServiceScriptExecution( config, this, this._logger, this._stopServices.promise, - adoptee + this._previousIterationServices?.get(key) ); if (config.isPersistent) { this._persistentServices.set(key, execution); diff --git a/src/logging/default-logger.ts b/src/logging/default-logger.ts index db6ae7379..959c11892 100644 --- a/src/logging/default-logger.ts +++ b/src/logging/default-logger.ts @@ -204,10 +204,10 @@ export class DefaultLogger implements Logger { console.error(`❌${prefix} Service exited unexpectedly`); break; } - case 'aborted': { - // This event isn't very useful to log. Things get aborted only - // because of a failure somewhere else, which should already get - // reported. + case 'aborted': + case 'dependency-service-exited-unexpectedly': { + // These event isn't very useful to log, because they are downstream + // of failures that already get reported elsewhere. break; } } diff --git a/src/test/service.test.ts b/src/test/service.test.ts index 338fe6a74..3753c30e3 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -87,7 +87,7 @@ test( await serviceInv.closed; await wireit.waitForLog(/Service stopped/); - await wireit.exit; + assert.equal((await wireit.exit).code, 0); assert.equal(service.numInvocations, 1); assert.equal(consumer.numInvocations, 1); }) @@ -170,7 +170,7 @@ test( await serviceDepInv.closed; await wireit.waitForLog(/\[serviceDep\] Service stopped/); - await wireit.exit; + assert.equal((await wireit.exit).code, 0); assert.equal(standardDep.numInvocations, 1); assert.equal(serviceDep.numInvocations, 1); assert.equal(service.numInvocations, 1); @@ -537,12 +537,58 @@ for (const failureMode of ['continue', 'no-new', 'kill']) { assert.not(service2Inv.isRunning); await wireit.waitForLog(/\[service2\] Service stopped/); - await wireit.exit; + assert.equal((await wireit.exit).code, 1); assert.equal(standard.numInvocations, 1); assert.equal(service1.numInvocations, 1); assert.equal(service2.numInvocations, 1); }) ); + + test( + `after one persistent service fails, other persistent services stop, ` + + `and wireit exits non-zero`, + // entrypoint + // / \ + // v v + // service1 service2 + // (fails) + timeout(async ({rig}) => { + const service1 = await rig.newCommand(); + const service2 = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + entrypoint: 'wireit', + service1: 'wireit', + service2: 'wireit', + }, + wireit: { + entrypoint: { + dependencies: ['service1', 'service2'], + }, + service1: { + command: service1.command, + service: true, + }, + service2: { + command: service2.command, + service: true, + }, + }, + }, + }); + + const wireit = rig.exec('npm run entrypoint', { + env: {WIREIT_FAILURES: failureMode}, + }); + const service1Inv = await service1.nextInvocation(); + const service2Inv = await service2.nextInvocation(); + service1Inv.exit(1); + await service1Inv.closed; + await service2Inv.closed; + assert.equal((await wireit.exit).code, 1); + }) + ); } test( @@ -672,10 +718,14 @@ test( } wireit.kill(); - await wireit.exit; + const {stdout} = await wireit.exit; assert.equal(service1.numInvocations, 1); assert.equal(service2.numInvocations, 1); assert.equal(standard.numInvocations, 2); + + // Check that we only print "Service started" when we *actually* start a + // service, and not when we adopt an existing one into a new iteration. + assert.equal([...stdout.matchAll(/Service started/g)].length, 2); }) ); diff --git a/src/test/util/test-rig.ts b/src/test/util/test-rig.ts index 5b4e7d931..ce72da46a 100644 --- a/src/test/util/test-rig.ts +++ b/src/test/util/test-rig.ts @@ -218,8 +218,10 @@ class ExecResult { private readonly _child: ChildProcessWithoutNullStreams; private readonly _exited = new Deferred(); private _running = true; - private _stdout = ''; - private _stderr = ''; + private _allStdout = ''; + private _allStderr = ''; + private _matcherStdout = ''; + private _matcherStderr = ''; constructor( command: string, @@ -269,8 +271,8 @@ class ExecResult { this._exited.resolve({ code, signal, - stdout: this._stdout, - stderr: this._stderr, + stdout: this._allStdout, + stderr: this._allStderr, }); }); @@ -346,7 +348,7 @@ class ExecResult { const {re, deferred} = matcher; // Use exec instead of match because otherwise if the user used the /g/ // flag, we'll get an array and can't access the index. - const stdoutMatch = re.exec(this._stdout); + const stdoutMatch = re.exec(this._matcherStdout); if (stdoutMatch !== null) { deferred.resolve(); this._logMatchers.delete(matcher); @@ -355,7 +357,7 @@ class ExecResult { stdoutMatch.index + stdoutMatch[0].length ); } else { - const stderrMatch = re.exec(this._stderr); + const stderrMatch = re.exec(this._matcherStderr); if (stderrMatch !== null) { deferred.resolve(); this._logMatchers.delete(matcher); @@ -367,15 +369,16 @@ class ExecResult { } } if (stdoutLastIndex > 0) { - this._stdout = this._stdout.slice(stdoutLastIndex); + this._matcherStdout = this._matcherStdout.slice(stdoutLastIndex); } if (stderrLastIndex > 0) { - this._stderr = this._stderr.slice(stderrLastIndex); + this._matcherStderr = this._matcherStderr.slice(stderrLastIndex); } } private readonly _onStdout = (chunk: string | Buffer) => { - this._stdout += chunk; + this._allStdout += chunk; + this._matcherStdout += chunk; if (process.env.SHOW_TEST_OUTPUT) { process.stdout.write(chunk); } @@ -383,7 +386,8 @@ class ExecResult { }; private readonly _onStderr = (chunk: string | Buffer) => { - this._stderr += chunk; + this._allStderr += chunk; + this._matcherStdout += chunk; if (process.env.SHOW_TEST_OUTPUT) { process.stdout.write(chunk); }