-
Notifications
You must be signed in to change notification settings - Fork 216
[WIP][Bugfix] Mid-build Retries should not cause spurious errors. #458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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<TransformNode | SourceNode, NodeWrappers>; | ||
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(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: promise based timeout helper There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: cancel timer during debounce period |
||
try { | ||
resolve(this.build()); | ||
} catch(e) { | ||
reject(e); | ||
} | ||
}, e.retryIn); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verify the new approach to use cancellation/retries to implement the debounce works There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider |
||
}) | ||
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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import CancelationError from './errors/cancelation'; | ||
|
||
export default class CancelationRequest { | ||
_pendingWork: Promise<void>; | ||
_canceling: Promise<void> | null; | ||
private _pendingWork: Promise<void>; | ||
private _canceling: Promise<void> | null; | ||
private _cancelationError = new CancelationError('Build Canceled'); | ||
|
||
constructor(pendingWork: Promise<void>) { | ||
this._pendingWork = pendingWork; // all | ||
|
@@ -15,19 +16,23 @@ export default class CancelationRequest { | |
|
||
throwIfRequested() { | ||
if (this.isCanceled) { | ||
throw new CancelationError('Build Canceled'); | ||
throw this._cancelationError; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: Add test |
||
} | ||
} | ||
|
||
then() { | ||
return this._pendingWork.then(...arguments); | ||
} | ||
|
||
cancel() { | ||
cancel(cancelationError?: CancelationError) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: Add test |
||
if (this._canceling) { | ||
return this._canceling; | ||
} | ||
|
||
if (cancelationError) { | ||
this._cancelationError = cancelationError; | ||
} | ||
|
||
this._canceling = this._pendingWork.catch(e => { | ||
if (CancelationError.isCancelationError(e)) { | ||
return; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import Cancelation from './cancelation'; | ||
export default class Retry extends Cancelation { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: add test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: retry limit |
||
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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: better error message |
||
} | ||
|
||
try { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: leave comment why this is appropriate over |
||
} catch(e) { | ||
reject(e); | ||
} | ||
}); | ||
|
||
this.instance.on('error', (error: any) => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement some sort of retry limit, to prevent infinite builds