REQUEST FOR COMMENTS: Graceful shutdown of active calls #255
Conversation
|
Please fix tests |
src/service.js
Outdated
| .reduce((p, fn) => p.then(fn), this.Promise.resolve()); | ||
| } | ||
| return new this.Promise((resolve, reject) => { | ||
| const timeout = setTimeout(reject, this.settings.gracefulStopTimeout || this.broker.options.gracefulStopTimeout); |
There was a problem hiding this comment.
Because it is an "internal" service settings, please follow the conventions and use the $ prefix. Please change it to this.settings.$gracefulStopTimeout
src/service.js
Outdated
| const contextDisposeCatch = (ret) => { | ||
| return this.Promise.reject(contextDispose(ret)); | ||
| }; | ||
| return innerAction.handler(ctx).then(contextDispose, contextDisposeCatch); |
There was a problem hiding this comment.
Please use the common .then().catch() syntax:
return innerAction.handler(ctx).then(contextDispose).catch(contextDisposeCatch);
src/service.js
Outdated
| * @memberof Service | ||
| */ | ||
| _getActiveContexts() { | ||
| return this.activeContexts || []; |
There was a problem hiding this comment.
Please use _ before local variable name because it can conflict with user's service variables. Use as this._activeContexts
|
I wonder if the transport benchmarks should be updated to include a test with and a test without context tracking. The common benchmark with the FakeTransporter showed a significantly lower reduction in performance. |
| @@ -978,6 +991,14 @@ class ServiceBroker { | |||
| }); | |||
| } | |||
|
|
|||
| // Remove the context from the active contexts list | |||
| if (ctx.trackedBy) { | |||
| p.then(res => { | |||
There was a problem hiding this comment.
It breaks the promise chain and throw Unhandled exception. The correct way is p = p.then.....
| @@ -1007,6 +1028,14 @@ class ServiceBroker { | |||
| if (ctx.timeout > 0 && p.timeout) | |||
| p = p.timeout(ctx.timeout); | |||
|
|
|||
| // Remove the context from the active contexts list | |||
| if (ctx.trackedBy) { | |||
| p.then(res => { | |||
Accidental close of #250