microsoft
Handle rejected promise
Description
Inside EpochTrackerWithRedemption, treesLatestDeferral.reject() is called without handling the rejected promise. This results in an un-intentional unhandledRejection event being triggered. This PR resolves this issue by handing the rejected promise.
Steps to Reproduce Bug and Validate Solution
A test was added to epochTestsWithRedemption.spec.ts ("Failed fetchAndParseAsJson should not trigger unhandled rejection event") that will reproduce this issue. Running this test with the original EpochTracker code will trigger the unhandledRejection event.
PR Checklist
Use the check-list below to ensure your branch is ready for PR. If the item is not applicable, leave it blank.
- [X ] I have updated the documentation accordingly.
- [X ] I have added tests to cover my changes.
- [X ] All new and existing tests passed.
- [ X] My code follows the code style of this project.
- [X ] I ran the lint checks which produced no new errors nor warnings for my changes.
- [X ] I have checked to ensure there aren't other open Pull Requests for the same update/change.
Does this introduce a breaking change?
- [ ] Yes
- [ X] No
Testing
You can reproduce the original error by commenting out the code changes in EpochTracker (line 408) and running the tests in epochTestsWithRedemption.spec.ts. This should result in a failed test due to an unhandledRejection event being triggered. You can then un-comment the lines in EpochTracker and re-run the tests. All tests should pass with no unhandledRejection events being triggered.
Any relevant logs or outputs
NA
Other information or known dependencies
NA
I've been pondering this for a while, and looking this over just now another approach occurs to me - If you step back and see what this treesLatestDeferral is actually used for, it's just tracking completion, not the result itself. So it works just as well if all codepaths call resolve(). I think renaming the field and doing this approach will better model the purpose, and rightly precludes any unhandledRejection since this Promise doesn't actually care about the result and will never reject.
I agree, I like this approach as well. After implementing it, one of the tests fails ("Requests should fail if joinSession call fails and the getLatest call also fails"). Calling this.treesLatestDeferral.reject(error) on line 431 will cause an error on line 460 in the Promise.race() for anything waiting on the promise. The way I understand it is if joinSession fails, it try and repeat only after waiting for treesLatest to succeed, if treesLatest fails, joinSession should fail without repeating (and throw the error). Rejecting on line 431 ensures that joinSession will not repeat and throw the correct error.
Ahhh you're right, I see it. Ok. I think I would lean towards doing the empty catch in the constructor, except it's kind of a pain in this case since you'd have to pass through all the constructor params to the base class. Your call.
It might be interesting to try to get a more realistic test too. I think the reason the existing tests don't hit it is because of the epochCallback.setCallback technique which puts the concurrent requests in the same JS turn (they're all microtasks). Maybe updating that to do a setTimeout as well would trigger the unhandled rejections, then you wouldn't need to add another test case at all.
There are a number of tests that are failing. When I run the tests in the command line via npm they fail. When I try to debug the tests in VSCode, they don't fail. If I run a failing tests by itself (by adding 'it.only') they pass.
Some of the tests are failing with the exception: "TypeError: Attempted to wrap default which is already wrapped". This seems to be coming from the Sinon package.
The tests all pass if I add the '--parallel' flag to the test script inside the package.json file. At this point I am not sure what exactly is making the tests fail. I can't tell if this is a test setup issue or something else.