Skip to content

Commit

Permalink
fix(cli): CLI timeout fetching notices prints "unreachable" branch er…
Browse files Browse the repository at this point in the history
…ror message (#20308)

fixes #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*
  • Loading branch information
Naumel authored Jul 18, 2022
1 parent 37a44d7 commit 7c4cd96
Showing 1 changed file with 12 additions and 16 deletions.
28 changes: 12 additions & 16 deletions packages/aws-cdk/lib/notices.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { ClientRequest } from 'http';
import * as https from 'https';
import * as path from 'path';
import * as fs from 'fs-extra';
Expand Down Expand Up @@ -108,9 +109,18 @@ export class WebsiteNoticeDataSource implements NoticeDataSource {
fetch(): Promise<Notice[]> {
const timeout = 3000;
return new Promise((resolve, reject) => {
let req: ClientRequest | undefined;

let timer = setTimeout(() => {
if (req) {
req.destroy(new Error('Request timed out'));
}
}, timeout);

timer.unref();

try {
const req = https.get('https://cli.cdk.dev-tools.aws.dev/notices.json',
{ timeout },
req = https.get('https://cli.cdk.dev-tools.aws.dev/notices.json',
res => {
if (res.statusCode === 200) {
res.setEncoding('utf8');
Expand Down Expand Up @@ -138,20 +148,6 @@ export class WebsiteNoticeDataSource implements NoticeDataSource {
}
});
req.on('error', reject);
req.on('timeout', () => {
// The 'timeout' event doesn't stop anything by itself, it just
// notifies that it has been long time since we saw bytes.
// In our case, we want to give up.
req.destroy(new Error('Request timed out'));
});

// It's not like I don't *trust* the 'timeout' event... but I don't trust it.
// Add a backup timer that will destroy the request after all.
// (This is at least necessary to make the tests pass, but that's probably because of 'nock'.
// It's not clear whether users will hit this).
setTimeout(() => {
req.destroy(new Error('Request timed out. You should never see this message; if you do, please let us know at https://github.com/aws/aws-cdk/issues'));
}, timeout + 200);
} catch (e) {
reject(new Error(`HTTPS 'get' call threw an error: ${e.message}`));
}
Expand Down

0 comments on commit 7c4cd96

Please sign in to comment.