From c5787b85d19223d92025b6a98522cd598c46ee29 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 4 Nov 2022 06:12:22 -0700 Subject: [PATCH 01/15] Check exit codes in service tests --- src/test/service.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/service.test.ts b/src/test/service.test.ts index 338fe6a74..7374fdd7c 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,7 +537,7 @@ 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); From 7651e2c3675cf3021d38f5463f1842a54741389f Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 4 Nov 2022 07:03:06 -0700 Subject: [PATCH 02/15] Add test for persistent service failure --- src/test/service.test.ts | 46 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/test/service.test.ts b/src/test/service.test.ts index 7374fdd7c..9b70d6c1d 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -543,6 +543,52 @@ for (const failureMode of ['continue', 'no-new', 'kill']) { 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( From f0673f4c94d434022f55c87b37de8ceabf2b2435 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 4 Nov 2022 07:10:30 -0700 Subject: [PATCH 03/15] Notice failures in persistent services --- src/cli.ts | 16 ++++++++++++++++ src/execution/service.ts | 1 + 2 files changed, 17 insertions(+) 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/execution/service.ts b/src/execution/service.ts index 0ffe2dc13..1b3e1370b 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -784,6 +784,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand Date: Fri, 4 Nov 2022 08:11:05 -0700 Subject: [PATCH 04/15] Preserve full stdout/stderr in overall test rig result --- src/test/util/test-rig.ts | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) 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); } From 910321e98a6728ca75c80075f2035911fed071dc Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 4 Nov 2022 08:11:26 -0700 Subject: [PATCH 05/15] When adopting a service, transition directly to started. This avoids logging "Service started" in the adopted case, and is also just simpler. --- src/execution/service.ts | 62 +++++++++++++++++++++++++--------------- src/test/service.test.ts | 6 +++- 2 files changed, 44 insertions(+), 24 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index 1b3e1370b..1cc6a2fd6 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -146,20 +146,27 @@ function unexpectedState(state: ServiceState) { * │ └───────┬───────┘ │ * │ │ │ * │ depsStarted ▼ - * │ │ ╭─╮ │ - * │ │ │ start │ - * │ ┌────▼──▼─┴┐ │ + * │ │ │ + * │ │ │ + * ▼ ╔══════▼═══════╗ │ + * │ ║ has adoptee? ╟───── yes ───╮ │ + * │ ╚══════╤═══════╝ │ │ + * │ │ │ │ + * │ no │ │ + * │ │ ╭─╮ ▼ │ + * │ │ │ start │ │ + * │ ┌────▼──▼─┴┐ │ │ * │ ╭◄─ abort ┤ STARTING ├──── startErr ──────►──────┤ - * │ │ └────┬────┬┘ │ - * │ │ │ │ │ + * │ │ └────┬────┬┘ │ │ + * │ │ │ │ │ │ * │ │ │ ╰─ depServiceExit ─►─╮ │ - * ▼ │ │ │ │ - * │ │ │ │ │ - * │ ▼ │ ▼ ▼ - * │ │ started │ │ - * │ │ │ ╭─╮ │ │ - * │ │ │ │ start │ │ - * │ │ ┌────▼─▼─┴┐ │ │ + * ▼ │ │ │ │ │ + * │ │ │ │ │ │ + * │ ▼ │ ▼ ▼ ▼ + * │ │ started │ │ │ + * │ │ ╭─╮ │ ╭─────────◄────────╯ │ │ + * │ │ start │ │ │ │ │ + * │ │ ┌▼─┴─▼──▼─┐ │ │ * │ ├◄─ abort ┤ STARTED ├── exit ────────────────────┤ * │ │ └──────┬─┬┘ │ │ * │ │ │ │ │ │ @@ -565,17 +572,26 @@ 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(); }); diff --git a/src/test/service.test.ts b/src/test/service.test.ts index 9b70d6c1d..3753c30e3 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -718,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); }) ); From d97939754b50017bb8a3c161a8c06788f432f0c1 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 4 Nov 2022 08:21:57 -0700 Subject: [PATCH 06/15] Remove comment about promise GC, since I believe it is now resolved --- src/execution/service.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index 1cc6a2fd6..de0cf0f2d 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -259,13 +259,6 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand Date: Sat, 5 Nov 2022 09:17:43 -0700 Subject: [PATCH 07/15] Add dependency-service-exited-unexpectedly event --- src/event.ts | 9 +++++++++ src/execution/service.ts | 3 +-- src/execution/standard.ts | 23 ++++++----------------- src/logging/default-logger.ts | 8 ++++---- 4 files changed, 20 insertions(+), 23 deletions(-) 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/service.ts b/src/execution/service.ts index de0cf0f2d..982bb16c4 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -637,8 +637,7 @@ 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/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; } } From 64ea5f360bbe1ff8fc84b0679c5b5a103a35639d Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sat, 5 Nov 2022 09:20:14 -0700 Subject: [PATCH 08/15] CHANGELOG fixes --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) 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 From c2cdcf331f27d77f810f8c56f703bb4446f9db30 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sat, 5 Nov 2022 09:29:23 -0700 Subject: [PATCH 09/15] Only need one failure for the start promise --- src/execution/base.ts | 2 +- src/execution/service.ts | 18 ++++++++---------- src/executor.ts | 2 +- 3 files changed, 10 insertions(+), 12 deletions(-) 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 982bb16c4..867744306 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; } | { @@ -534,19 +534,17 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand Date: Sat, 5 Nov 2022 09:36:35 -0700 Subject: [PATCH 10/15] Make the start() promise fail if it terminates before starting --- src/execution/service.ts | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index 867744306..615fc66e8 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -508,12 +508,13 @@ 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, }; @@ -523,6 +524,27 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { 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; } case 'depsStarting': From 7820c78d0fd5ffa8ecd572c8eae8e0ecaad872d0 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sat, 5 Nov 2022 09:42:33 -0700 Subject: [PATCH 11/15] Add handling for dependency service start error --- src/execution/service.ts | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index 615fc66e8..eeee3f1bc 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -518,8 +518,12 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { - this._onDepsStarted(); + void this._startServices().then((result) => { + if (result.ok) { + this._onDepsStarted(); + } else { + this._onDepStartErr(result); + } }); void this._anyServiceTerminated.then(() => { this._onDepServiceExit(); @@ -648,6 +652,37 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand Date: Sat, 5 Nov 2022 09:43:48 -0700 Subject: [PATCH 12/15] Enter failed state on dependency exec error --- src/execution/service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/execution/service.ts b/src/execution/service.ts index eeee3f1bc..c0a773abf 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -386,6 +386,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand Date: Sat, 5 Nov 2022 10:09:05 -0700 Subject: [PATCH 13/15] Shut down floating previous services when aborting --- src/executor.ts | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/src/executor.ts b/src/executor.ts index c1562d304..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(); + } + } } /** @@ -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); From 68bd86153eca5a15c356e1dea38c624980c9c82f Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sat, 5 Nov 2022 10:16:14 -0700 Subject: [PATCH 14/15] Kill scripts if they start after a failure/abort --- src/execution/service.ts | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index c0a773abf..d96a89657 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -59,10 +59,14 @@ type ServiceState = child: ScriptChildProcess; fingerprint: Fingerprint; } - | {id: 'stopping'} + | { + id: 'stopping'; + child: ScriptChildProcess; + } | {id: 'stopped'} | { id: 'failing'; + child: ScriptChildProcess; failure: Failure; } | { @@ -690,6 +694,7 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand Date: Sat, 5 Nov 2022 10:19:12 -0700 Subject: [PATCH 15/15] Fix some issues around _onDepServiceExit --- src/execution/service.ts | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index d96a89657..952b8870e 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -530,9 +530,6 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { - this._onDepServiceExit(); - }); void this.terminated.then((result) => { if (started.settled) { return; @@ -633,6 +630,9 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand { + this._onDepServiceExit(); + }); return; } case 'failed': { @@ -703,7 +703,22 @@ export class ServiceScriptExecution extends BaseExecutionWithCommand