fix(webex-core): handle broken chain of promises#4873
fix(webex-core): handle broken chain of promises#4873mkesavan13 wants to merge 3 commits intowebex:nextfrom
Conversation
mkesavan13
left a comment
There was a problem hiding this comment.
Leaving notes of my findings with Cursor as co-pilot.
| this.prepareItem(item) | ||
| .then((req) => { | ||
| defer.promise = defer.promise | ||
| .then(tap(() => this.deferreds.delete(idx))) |
There was a problem hiding this comment.
Here's the definition of tap:
function tap(fn) {
return function (r) {
return new _promise.default(function (resolve) {
resolve(fn(r));
}).then(function () {
return r;
}).catch(function () {
return r;
});
};
}
It does nothing but return a curried function that returns a promise which executes and tries to resolve passed fn which in this case is () => this.deferreds.delete(idx)
If that's resolved, resolves with idx as value or when rejected, rejects with idx as value again.
Per the old flow, this was required to catch again to do the deletion.
Now, we have a cleaner code with promise.then receiving methods to resolve/reject directly
| return this.handleHttpError(reason).catch(() => | ||
| Promise.all( | ||
| queue.map((item) => | ||
| this.getDeferredForRequest(item).then((defer) => { | ||
| defer.reject(reason); | ||
| }) | ||
| ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Although handleHttpError is part of batcher and could have made this change directly there, the handleHttpError is overridden by downstream classes like avatar or metrics plugin. Therefore, this fix would get lost for such plugins.
If we add it in the executeQueue's catch block, the fix is propagated to all the downstream classes as executeQueue is nowhere overridden.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
akulakum
left a comment
There was a problem hiding this comment.
This looks like a solid fix for the unhandledRejection / unsettled-queue behavior described in the issue and demo: cleaning up the deferred promise handling, rejecting queued work when handleHttpError fails (including GET-style cases without a body), and avoiding a re-thrown rejection on a debounced path that nothing awaits. The unit tests for unhandledRejection and for failing queued deferreds are a good addition.
Before merge, could we confirm two things for maintainers and downstream consumers? First, with the final executeQueue catch now swallowing the rejection after logging, are we certain that every path that reaches that catch has already failed the relevant deferred promises, so we do not risk leaving a caller hanging? Second, for batcher subclasses that override handleHttpError, does the new fallback in executeQueue still guarantee that the full queue is rejected when appropriate?
COMPLETES SPARK-747471
This pull request addresses
Issue #4588
Basically, the SDK was internally having an unhandled promise rejection that affected the flow of external apps.
by making the following changes
Restricted Cisco Only Vidcast for the fix: https://app.vidcast.io/share/69133bb4-84c1-4b08-a4cf-b0db6f05a233
Change Type
The following scenarios were tested
Ran this snippet in Meetings Sample to not see the
unhandledRejectionhandler being called in the following cases:The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.