[WIP][Bugfix] Mid-build Retries should not cause spurious errors. #458
Conversation
stefanpenner
commented
Mar 27, 2020
•
stefanpenner
commented
- fix todos
- cleanup
- retry limit
- add additional tests
| if (RetryCancellationRequest.isRetry(e)) { | ||
| this._cancelationRequest = undefined; | ||
| await new Promise((resolve, reject) => { | ||
| setTimeout(() => { |
There was a problem hiding this comment.
TODO: promise based timeout helper
There was a problem hiding this comment.
TODO: cancel timer during debounce period
| try { | ||
| await pipeline; | ||
| this.buildHeimdallTree(this.outputNodeWrapper); | ||
| } catch (e) { | ||
| if (RetryCancellationRequest.isRetry(e)) { |
There was a problem hiding this comment.
Implement some sort of retry limit, to prevent infinite builds
| } catch(e) { | ||
| reject(e); | ||
| } | ||
| }, e.retryIn); |
There was a problem hiding this comment.
Verify the new approach to use cancellation/retries to implement the debounce works
There was a problem hiding this comment.
consider retryIn -> debounce ?
| @@ -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.
TODO: Add test
| } | ||
| } | ||
|
|
||
| then() { | ||
| return this._pendingWork.then(...arguments); | ||
| } | ||
|
|
||
| cancel() { | ||
| cancel(cancelationError?: CancelationError) { |
There was a problem hiding this comment.
TODO: Add test
| @@ -245,9 +230,16 @@ class Watcher extends EventEmitter { | |||
|
|
|||
| this._quittingPromise = (async () => { | |||
| try { | |||
| await this.watcherAdapter.quit(); | |||
| await Promise.all([ | |||
| this.builder.cancel(), | |||
There was a problem hiding this comment.
TODO: verify this doesn't throw under normal operation.
| } finally { | ||
| try { | ||
| if (this._nextBuild) { |
There was a problem hiding this comment.
TODO: this is yucky
test/builder_test.js
Outdated
| @@ -1027,7 +1027,7 @@ describe('Builder', function() { | |||
| }); | |||
|
|
|||
| describe('heimdall stats', function() { | |||
| it('produces stats', async function() { | |||
| it.skip('produces stats', async function() { | |||
test/watcher_test.js
Outdated
| @@ -122,7 +122,7 @@ describe('Watcher', function() { | |||
| }); | |||
|
|
|||
| describe('change', function() { | |||
| it('on change, rebuild is invoked', async function() { | |||
| it.skip('on change, rebuild is invoked', async function() { | |||
| @@ -208,6 +209,7 @@ describe('Watcher', function() { | |||
| await first.resolve(); | |||
| await second.resolve(); | |||
|
|
|||
| await firstBuild; | |||
There was a problem hiding this comment.
Adding this, reproduces the regression that motivated this fix
TODO: leave comment/explicit test so future travelers understand the importance of this.
| @@ -199,8 +164,28 @@ class Watcher extends EventEmitter { | |||
| logger.debug('buildFailure'); | |||
| this.emit('buildFailure', err); | |||
| } | |||
| ); | |||
| return buildPromise; | |||
| ).finally(() => this._activeBuild = false); | |||
There was a problem hiding this comment.
TODO: technically we support node 8 still, so this needs to use syntax finally
|
Sorry for the conflicts, will need to rebase on top of #459 and revert the revert (I think). |
|
@rwjblue yup, thats reasonable. I'll incorporate the revert revert in this PR shortly |
| if (this._nextBuild) { | ||
| return this._nextBuild.promise; | ||
| } else { | ||
| return this._currentBuild; |
There was a problem hiding this comment.
consider .then() so that external unhandled rejections occure.
