From e9e4befa03102db08fb70e804a3e7127b3e7cd9a Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Fri, 27 Mar 2020 17:10:59 -0600 Subject: [PATCH] [WIP][Bugfix] Mid-build Retries should not cause spurious errors. --- lib/builder.ts | 37 +++++- lib/cancelation-request.ts | 13 ++- lib/errors/retry-cancelation.ts | 14 +++ lib/middleware.ts | 4 +- lib/server.ts | 8 +- lib/watcher.ts | 194 +++++++++++++++----------------- test/watcher_test.js | 2 + 7 files changed, 160 insertions(+), 112 deletions(-) create mode 100644 lib/errors/retry-cancelation.ts diff --git a/lib/builder.ts b/lib/builder.ts index aa31740f..9d381981 100644 --- a/lib/builder.ts +++ b/lib/builder.ts @@ -8,6 +8,7 @@ import BuilderError from './errors/builder'; import NodeSetupError from './errors/node-setup'; import BuildError from './errors/build'; import CancelationRequest from './cancelation-request'; +import RetryCancellationRequest from './errors/retry-cancelation'; import filterMap from './utils/filter-map'; import { EventEmitter } from 'events'; import { TransformNode, SourceNode, Node } from 'broccoli-node-api'; @@ -53,7 +54,7 @@ class Builder extends EventEmitter { watchedPaths: string[]; _nodeWrappers: Map; outputNodeWrapper: NodeWrappers; - _cancelationRequest: any; + _cancelationRequest?: CancelationRequest; outputPath: string; buildId: number; builderTmpDir!: string; @@ -143,8 +144,12 @@ class Builder extends EventEmitter { // // 1. build up a promise chain, which represents the complete build pipeline = pipeline.then(async () => { + if (this._cancelationRequest) { + this._cancelationRequest.throwIfRequested(); + } else { + throw new BuilderError('Broccoli: The current build is missing a CancelationRequest, this is unexpected please report an issue: https://github.com/broccolijs/broccoli/issues/new '); + } // 3. begin next build step - this._cancelationRequest.throwIfRequested(); this.emit('beginNode', nw); try { await nw.build(); @@ -166,17 +171,35 @@ class Builder extends EventEmitter { // cleanup, or restarting the build itself. this._cancelationRequest = new CancelationRequest(pipeline); + let skipFinally = false; try { await pipeline; this.buildHeimdallTree(this.outputNodeWrapper); + } catch (e) { + if (RetryCancellationRequest.isRetry(e)) { + this._cancelationRequest = undefined; + await new Promise((resolve, reject) => { + setTimeout(() => { + try { + resolve(this.build()); + } catch(e) { + reject(e); + } + }, e.retryIn); + }) + skipFinally = true; + } else { + throw e; + } } finally { + if (skipFinally) { return; } let buildsSkipped = filterMap( this._nodeWrappers.values(), (nw: NodeWrappers) => nw.buildState.built === false ).length; logger.debug(`Total nodes skipped: ${buildsSkipped} out of ${this._nodeWrappers.size}`); - this._cancelationRequest = null; + this._cancelationRequest = undefined; } } @@ -186,6 +209,14 @@ class Builder extends EventEmitter { } } + async retry(retryIn: number) { + if (this._cancelationRequest) { + return this._cancelationRequest.cancel(new RetryCancellationRequest('Retry', retryIn)); + } else { + return this.build(); + } + } + // Destructor-like method. Waits on current node to finish building, then cleans up temp directories async cleanup() { try { diff --git a/lib/cancelation-request.ts b/lib/cancelation-request.ts index 903f894b..8d213f65 100644 --- a/lib/cancelation-request.ts +++ b/lib/cancelation-request.ts @@ -1,8 +1,9 @@ import CancelationError from './errors/cancelation'; export default class CancelationRequest { - _pendingWork: Promise; - _canceling: Promise | null; + private _pendingWork: Promise; + private _canceling: Promise | null; + private _cancelationError = new CancelationError('Build Canceled'); constructor(pendingWork: Promise) { this._pendingWork = pendingWork; // all @@ -15,7 +16,7 @@ export default class CancelationRequest { throwIfRequested() { if (this.isCanceled) { - throw new CancelationError('Build Canceled'); + throw this._cancelationError; } } @@ -23,11 +24,15 @@ export default class CancelationRequest { return this._pendingWork.then(...arguments); } - cancel() { + cancel(cancelationError?: CancelationError) { if (this._canceling) { return this._canceling; } + if (cancelationError) { + this._cancelationError = cancelationError; + } + this._canceling = this._pendingWork.catch(e => { if (CancelationError.isCancelationError(e)) { return; diff --git a/lib/errors/retry-cancelation.ts b/lib/errors/retry-cancelation.ts new file mode 100644 index 00000000..05699b8e --- /dev/null +++ b/lib/errors/retry-cancelation.ts @@ -0,0 +1,14 @@ +import Cancelation from './cancelation'; +export default class Retry extends Cancelation { + isRetry: boolean = true; + retryIn: Number; + + static isRetry(e: any): boolean { + return typeof e === 'object' && e !== null && e.isRetry === true; + } + + constructor(message = 'Retry', retryIn: number) { + super(message); + this.retryIn = retryIn; + } +} diff --git a/lib/middleware.ts b/lib/middleware.ts index 9db907f3..3a73a8e4 100644 --- a/lib/middleware.ts +++ b/lib/middleware.ts @@ -141,8 +141,8 @@ export = function getMiddleware(watcher: Watcher, options: MiddlewareOptions = { const outputPath = path.resolve(watcher.builder.outputPath); return async function broccoliMiddleware(request: any, response: any, next: any) { - if (watcher.currentBuild == null) { - throw new Error('Waiting for initial build to start'); + if (!watcher.currentBuild) { + throw new Error('Broccoli: watcher must have a currentBuild'); } try { diff --git a/lib/server.ts b/lib/server.ts index 90821778..19f240cd 100644 --- a/lib/server.ts +++ b/lib/server.ts @@ -86,8 +86,12 @@ class Server extends EventEmitter { this.instance.listen(this._port, this._host); this.instance.on('listening', () => { - this.ui.writeLine(`Serving on ${this._url}\n`); - resolve(this._watcher.start()); + try { + this.ui.writeLine(`Serving on ${this._url}\n`); + this._watcher.start().then(resolve, reject); + } catch(e) { + reject(e); + } }); this.instance.on('error', (error: any) => { diff --git a/lib/watcher.ts b/lib/watcher.ts index cc8013c0..bcc35ad8 100644 --- a/lib/watcher.ts +++ b/lib/watcher.ts @@ -15,20 +15,34 @@ interface WatcherOptions { // WatcherAdapter handles I/O via the sane package, and could be pluggable in // principle. +type Deferred = { + promise: Promise; + resolve: (value?: any) => void; + reject: (error?: any) => void; +} + +function deferred() : Deferred { + const deferred = {} as Deferred; + deferred.promise = new Promise((resolve, reject) => { + deferred.resolve = resolve; + deferred.reject = reject ; + }); + + return deferred; +} + class Watcher extends EventEmitter { - _changedFiles: string[]; - _quitting?: boolean; // is this ever set - _rebuildScheduled: boolean; - _ready: boolean; - _quittingPromise: Promise | null; - _lifetime: { - promise?: Promise; - resolve?: (value: any) => void; - reject?: (error: any) => void; - } | null; + private _changedFiles: string[] = []; + private _quitting?: boolean; // is this ever set + private _rebuildScheduled = false; + private _ready = false; + private _quittingPromise: Promise | null = null; + private _lifetime: Deferred | null = null; + private _nextBuild? = deferred(); + private _currentBuild?: Promise; + private _activeBuild = false; options: WatcherOptions; - _currentBuild: Promise; watcherAdapter: WatcherAdapter; builder: any; @@ -38,42 +52,20 @@ class Watcher extends EventEmitter { if (this.options.debounce == null) { this.options.debounce = 100; } - this._currentBuild = Promise.resolve(); this.builder = builder; this.watcherAdapter = this.options.watcherAdapter || new WatcherAdapter(watchedNodes, this.options.saneOptions); - - this._rebuildScheduled = false; - this._ready = false; - this._quittingPromise = null; - this._lifetime = null; - this._changedFiles = []; + if (this._nextBuild) { + this._nextBuild.promise.catch(() => {}); + } } get currentBuild() { - return this._currentBuild; - } - - // TODO: this is an interim solution, pending a largely cleanup of this class. - // Currently I can rely on understanding the various pieces of this class, to - // know this is safe. This is not a good long term solution, but given - // relatively little time to address this currently, it is "ok". I do plan, - // as time permits to circle back, and do a more thorough refactoring of this - // class, to ensure it is safe for future travelers. - private _updateCurrentBuild(promise: Promise) { - this._currentBuild = promise; - - promise.catch(() => { - /** - * The watcher internally follows currentBuild, and outputs errors appropriately. - * Since watcher.currentBuild is public API, we must allow public follows - * to still be informed of rejections. However we do not want `_currentBuild` itself - * to trigger unhandled rejections. - * - * By catching errors here, but returning `promise` instead of the chain from - * `promise.catch`, both goals are accomplished. - */ - }); + if (this._nextBuild) { + return this._nextBuild.promise; + } else { + return this._currentBuild; + } } start() { @@ -81,29 +73,28 @@ class Watcher extends EventEmitter { throw new Error('Watcher.prototype.start() must not be called more than once'); } - this._lifetime = {}; - let lifetime = this._lifetime; - lifetime.promise = new Promise((resolve, reject) => { - lifetime.resolve = resolve; - lifetime.reject = reject; - }); + this._lifetime = deferred(); + this._lifetime.promise.catch(() => {}); this.watcherAdapter.on('change', this._change.bind(this)); this.watcherAdapter.on('error', this._error.bind(this)); - this._updateCurrentBuild( - (async () => { - try { - await this.watcherAdapter.watch(); - logger.debug('ready'); - this.emit('ready'); - this._ready = true; - } catch (e) { - this._error(e); - } - return this._build(); - })() - ); + (async () => { + try { + await this.watcherAdapter.watch(); + logger.debug('ready'); + this.emit('ready'); + this._ready = true; + } catch (e) { + this._error(e); + } + + try { + await this._build(); + } catch (e) { + // _build handles error reporting internally + } + })() return this._lifetime.promise; } @@ -118,63 +109,37 @@ class Watcher extends EventEmitter { logger.debug('change', 'ignored: before ready'); return; } - if (this._rebuildScheduled) { - logger.debug('change', 'ignored: rebuild scheduled already'); - return; - } logger.debug('change', event, filePath, root); this.emit('change', event, filePath, root); - this._rebuildScheduled = true; - - try { - // cancel the current builder and wait for it to finish the current plugin - await this.builder.cancel(); - - // Wait for current build, and ignore build failure - await this.currentBuild; - } catch (e) { - /* we don't care about failures in the last build, simply start the - * next build once the last build has completed - * */ - } if (this._quitting) { - this._updateCurrentBuild(Promise.resolve()); - return; + await this.currentBuild; + } else if (this._activeBuild) { + await this.builder.retry(this.options.debounce); + } else { + try { + await this._build(path.join(root, filePath)); + } catch (e) { + // _build handles error reporting internally + } } - - this._updateCurrentBuild( - new Promise((resolve, reject) => { - logger.debug('debounce'); - this.emit('debounce'); - setTimeout(() => { - // Only set _rebuildScheduled to false *after* the setTimeout so that - // change events during the setTimeout don't trigger a second rebuild - try { - this._rebuildScheduled = false; - resolve(this._build(path.join(root, filePath))); - } catch (e) { - reject(e); - } - }, this.options.debounce); - }) - ); } - _build(filePath?: string): Promise { + async _build(filePath?: string) : Promise { logger.debug('buildStart'); this.emit('buildStart'); const start = process.hrtime(); // This is to maintain backwards compatibility with broccoli-sane-watcher - let annotation = { + const annotation = { type: filePath ? 'rebuild' : 'initial', reason: 'watcher', primaryFile: filePath, changedFiles: this._changedFiles, }; + this._activeBuild = true; const buildPromise = this.builder.build(null, annotation); // Trigger change/error events. Importantly, if somebody else chains to // currentBuild, their callback will come after our events have @@ -199,8 +164,28 @@ class Watcher extends EventEmitter { logger.debug('buildFailure'); this.emit('buildFailure', err); } - ); - return buildPromise; + ).finally(() => this._activeBuild = false); + + + if (this._nextBuild) { + this._nextBuild.resolve(buildPromise); + } + this._currentBuild = buildPromise; + this._nextBuild = undefined; + + buildPromise.catch(() => { + /** + * The watcher internally follows currentBuild, and outputs errors appropriately. + * Since watcher.currentBuild is public API, we must allow public follows + * to still be informed of rejections. However we do not want `_currentBuild` itself + * to trigger unhandled rejections. + * + * By catching errors here, but returning `promise` instead of the chain from + * `promise.catch`, both goals are accomplished. + */ + }); + + return this._currentBuild; } async _error(err: any) { @@ -245,9 +230,16 @@ class Watcher extends EventEmitter { this._quittingPromise = (async () => { try { - await this.watcherAdapter.quit(); + await Promise.all([ + this.builder.cancel(), + this.watcherAdapter.quit(), + ]) + } catch (e) { } finally { try { + if (this._nextBuild) { + this._nextBuild.resolve(); + } await this.currentBuild; } catch (e) { // Wait for current build, and ignore build failure diff --git a/test/watcher_test.js b/test/watcher_test.js index 09dfb897..ee6e7e0e 100644 --- a/test/watcher_test.js +++ b/test/watcher_test.js @@ -192,6 +192,7 @@ describe('Watcher', function() { watcher.start(); + const firstBuild = watcher.currentBuild; await watcher.ready(); { const changedBuild = watcher._change('change', 'foo.js', 'root'); @@ -208,6 +209,7 @@ describe('Watcher', function() { await first.resolve(); await second.resolve(); + await firstBuild; await watcher.currentBuild; expect(first.buildCount).to.eq(2);