From 9142be53a075ecaef8b8ad0375647c38760da90d Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Wed, 19 Oct 2022 10:39:14 -0700 Subject: [PATCH 1/5] Remove redundant throw --- src/watcher.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 23932d7cff441f69e452e4c9e3e66cfa73b0f424 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Wed, 19 Oct 2022 08:45:50 -0700 Subject: [PATCH 2/5] Add BaseExecutionWithCommand base class --- src/execution/base.ts | 9 +++++++++ src/execution/service.ts | 4 ++-- src/execution/standard.ts | 4 ++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/execution/base.ts b/src/execution/base.ts index 946411aa3..3e638a963 100644 --- a/src/execution/base.ts +++ b/src/execution/base.ts @@ -84,3 +84,12 @@ 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 {} diff --git a/src/execution/service.ts b/src/execution/service.ts index b2e91dfec..8e1cea8c8 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -4,7 +4,7 @@ * 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'; @@ -13,7 +13,7 @@ import type {ServiceScriptConfig} from '../config.js'; /** * Execution for a {@link ServiceScriptConfig}. */ -export class ServiceScriptExecution extends BaseExecution { +export class ServiceScriptExecution extends BaseExecutionWithCommand { /** * 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..3b89c9e3c 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; From 92b91f804162990f47e75126997138c4a9e0435c Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 21 Oct 2022 10:18:48 -0700 Subject: [PATCH 3/5] Add servicesNotNeeded property --- src/execution/base.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/execution/base.ts b/src/execution/base.ts index 3e638a963..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'; @@ -92,4 +93,13 @@ export abstract class BaseExecutionWithCommand< T extends ScriptConfig & { command: Exclude; } -> extends BaseExecution {} +> 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; +} From 649ff19dc4303e8ee7ae5e856935d23e8ee75b70 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Fri, 21 Oct 2022 10:19:13 -0700 Subject: [PATCH 4/5] Resolve servicesNotNeeded in standard scripts --- src/execution/standard.ts | 98 ++++++++++++++++++++++----------------- 1 file changed, 55 insertions(+), 43 deletions(-) diff --git a/src/execution/standard.ts b/src/execution/standard.ts index 3b89c9e3c..920303d44 100644 --- a/src/execution/standard.ts +++ b/src/execution/standard.ts @@ -60,57 +60,61 @@ export class StandardScriptExecution extends BaseExecutionWithCommand { - 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 BaseExecutionWithCommand { + // 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 BaseExecutionWithCommand Date: Fri, 21 Oct 2022 10:21:56 -0700 Subject: [PATCH 5/5] Pass abort promise down to services --- src/execution/service.ts | 12 ++++++++++++ src/executor.ts | 9 ++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/execution/service.ts b/src/execution/service.ts index 8e1cea8c8..499d6885c 100644 --- a/src/execution/service.ts +++ b/src/execution/service.ts @@ -9,11 +9,23 @@ 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 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/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,