fix(cli): CLI timeout fetching notices prints "unreachable" branch error message#20308
fix(cli): CLI timeout fetching notices prints "unreachable" branch error message#20308mergify[bot] merged 13 commits intoaws:mainfrom Naumel:master
Conversation
|
|
packages/aws-cdk/lib/notices.ts
Outdated
| return new Promise((resolve, reject) => { | ||
|
|
||
| var timer = setTimeout(() => { | ||
| reject(new Error('Request timed out.')); |
There was a problem hiding this comment.
We must call req.destroy, otherwise the socket will continue to do work in the background (causing node to not exit) even though we've stopped waiting for the result.
packages/aws-cdk/lib/notices.ts
Outdated
| } catch (e) { | ||
| reject(new Error(`HTTPS 'get' call threw an error: ${e.message}`)); | ||
| } finally { | ||
| clearTimeout(timer); |
There was a problem hiding this comment.
Since the finally will execute immediately after starting the async process, we'll immediately cancel the timer and the timeout will never fire. The unref by itself should do it.
rix0rrr
left a comment
There was a problem hiding this comment.
Two style nitpicks on the PR itself:
- Our PR titles go into the CHANGELOG. Therefore, name them in a way that makes sense for people to read there. It will go under the heading "Bug fixes", so describe the bug that is fixed. For example: "CLI timeout fetching notices prints scary message"
- Please use the PR body to describe what the bug was, what you did to fix it, and alternatives you considered but rejected (if applicable). Some extra context setting makes it easier to review, and because of our distributedness overcommunicating is usually better than undercommunicating.
|
@Naumel If you change the PR description to say (That's how github suggests we link issues, but i guess it still picks up other ways too. Unfortunately, the github action I wrote is not as robust...) |
|
There is a test already checking for the timeout scenario, unless there's a suggestion for a more granular scenario I can write, I'll leave the existing tests as they are. |
|
Conditionally approved. |
|
@Naumel I added the |
|
I am confident and will deal with the consequences :) |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…ror message (aws#20308) fixes aws#20069 Problem: the [error message](https://github.com/aws/aws-cdk/blob/fd306ee05cfa7ebaa8d997007500d89d62868897/packages/aws-cdk/lib/notices.ts#L148-L154) should not be displayed. It was part of a backup timer acting as a fail-safe. Additionally, the timer from `setTimeout()` is not cleared. the issue is being addressed by invoking `unref()`. Overall, the behavior is correct: the request is being terminated, but the error message prompted the users encountering it to open an issue. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
fixes #20069
Problem: the error message should not be displayed. It was part of a backup timer acting as a fail-safe.
Additionally, the timer from
setTimeout()is not cleared. the issue is being addressed by invokingunref().Overall, the behavior is correct: the request is being terminated, but the error message prompted the users encountering it to open an issue.
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license