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); }