From a691a2dd6eb354e42a22ed28ede2be8ce5ebf317 Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Mon, 28 Feb 2022 17:00:36 +0000 Subject: [PATCH 1/2] fix(cli): long connection timeout slows the CLI down --- packages/aws-cdk/lib/notices.ts | 69 ++++++++++++++++----------- packages/aws-cdk/test/notices.test.ts | 35 +++++++++++++- 2 files changed, 74 insertions(+), 30 deletions(-) diff --git a/packages/aws-cdk/lib/notices.ts b/packages/aws-cdk/lib/notices.ts index 6b9131af6f258..3169de43e109a 100644 --- a/packages/aws-cdk/lib/notices.ts +++ b/packages/aws-cdk/lib/notices.ts @@ -105,37 +105,46 @@ export interface NoticeDataSource { export class WebsiteNoticeDataSource implements NoticeDataSource { fetch(): Promise { + const timeout = 1000; + return new Promise((resolve) => { try { - const req = https.get('https://cli.cdk.dev-tools.aws.dev/notices.json', res => { - if (res.statusCode === 200) { - res.setEncoding('utf8'); - let rawData = ''; - res.on('data', (chunk) => { - rawData += chunk; - }); - res.on('end', () => { - try { - const data = JSON.parse(rawData).notices as Notice[]; - resolve(data ?? []); - } catch (e) { - debug(`Failed to parse notices: ${e}`); + const req = https.get('https://cli.cdk.dev-tools.aws.dev/notices.json', + { timeout }, + res => { + const startTime = Date.now(); + if (res.statusCode === 200) { + res.setEncoding('utf8'); + let rawData = ''; + res.on('data', (chunk) => { + if (Date.now() - startTime > timeout) { + resolve([]); + } + rawData += chunk; + }); + res.on('end', () => { + try { + const data = JSON.parse(rawData).notices as Notice[]; + resolve(data ?? []); + } catch (e) { + debug(`Failed to parse notices: ${e}`); + resolve([]); + } + }); + res.on('error', e => { + debug(`Failed to fetch notices: ${e}`); resolve([]); - } - }); - res.on('error', e => { - debug(`Failed to fetch notices: ${e}`); + }); + } else { + debug(`Failed to fetch notices. Status code: ${res.statusCode}`); resolve([]); - }); - } else { - debug(`Failed to fetch notices. Status code: ${res.statusCode}`); - resolve([]); - } - }); + } + }); req.on('error', e => { debug(`Error on request: ${e}`); resolve([]); }); + req.on('timeout', _ => resolve([])); } catch (e) { debug(`HTTPS 'get' call threw an error: ${e}`); resolve([]); @@ -176,14 +185,18 @@ export class CachedDataSource implements NoticeDataSource { } private async load(): Promise { + const defaultValue = { + expiration: 0, + notices: [], + }; + try { - return await fs.readJSON(this.fileName) as CachedNotices; + return fs.existsSync(this.fileName) + ? await fs.readJSON(this.fileName) as CachedNotices + : defaultValue; } catch (e) { debug(`Failed to load notices from cache: ${e}`); - return { - expiration: 0, - notices: [], - }; + return defaultValue; } } diff --git a/packages/aws-cdk/test/notices.test.ts b/packages/aws-cdk/test/notices.test.ts index 47541c6a709a9..9dba8a45dd7ab 100644 --- a/packages/aws-cdk/test/notices.test.ts +++ b/packages/aws-cdk/test/notices.test.ts @@ -12,6 +12,7 @@ import { WebsiteNoticeDataSource, } from '../lib/notices'; import * as version from '../lib/version'; +import * as logging from '../lib/logging'; const BASIC_NOTICE = { title: 'Toggling off auto_delete_objects for Bucket empties the bucket', @@ -233,6 +234,32 @@ describe('cli notices', () => { expect(result).toEqual([]); }); + test('returns empty array when the connection stays idle for too long', async () => { + nock('https://cli.cdk.dev-tools.aws.dev') + .get('/notices.json') + .delayConnection(2000) + .reply(200, { + notices: [BASIC_NOTICE], + }); + + const result = await dataSource.fetch(); + + expect(result).toEqual([]); + }); + + test('returns empty array when the request takes too long to finish', async () => { + nock('https://cli.cdk.dev-tools.aws.dev') + .get('/notices.json') + .delayBody(2000) + .reply(200, { + notices: [BASIC_NOTICE], + }); + + const result = await dataSource.fetch(); + + expect(result).toEqual([]); + }); + function mockCall(statusCode: number, body: any): Promise { nock('https://cli.cdk.dev-tools.aws.dev') .get('/notices.json') @@ -284,12 +311,16 @@ describe('cli notices', () => { }); test('retrieves data from the delegate when the file cannot be read', async () => { - const nonExistingFile = path.join(os.tmpdir(), 'cache.json'); - const dataSource = dataSourceWithDelegateReturning(freshData, nonExistingFile); + const debugSpy = jest.spyOn(logging, 'debug'); + + const dataSource = dataSourceWithDelegateReturning(freshData, 'does-not-exist.json'); const notices = await dataSource.fetch(); expect(notices).toEqual(freshData); + expect(debugSpy).not.toHaveBeenCalled(); + + debugSpy.mockRestore(); }); test('retrieved data from the delegate when it is configured to ignore the cache', async () => { From 5a8ff805524f131c675343f28acdf6f64d7a8cae Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Mon, 28 Feb 2022 17:05:33 +0000 Subject: [PATCH 2/2] timeout = 3 seconds --- packages/aws-cdk/lib/notices.ts | 2 +- packages/aws-cdk/test/notices.test.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/aws-cdk/lib/notices.ts b/packages/aws-cdk/lib/notices.ts index 3169de43e109a..83b38412aa87b 100644 --- a/packages/aws-cdk/lib/notices.ts +++ b/packages/aws-cdk/lib/notices.ts @@ -105,7 +105,7 @@ export interface NoticeDataSource { export class WebsiteNoticeDataSource implements NoticeDataSource { fetch(): Promise { - const timeout = 1000; + const timeout = 3000; return new Promise((resolve) => { try { diff --git a/packages/aws-cdk/test/notices.test.ts b/packages/aws-cdk/test/notices.test.ts index 9dba8a45dd7ab..c2efe5cfaf904 100644 --- a/packages/aws-cdk/test/notices.test.ts +++ b/packages/aws-cdk/test/notices.test.ts @@ -3,6 +3,7 @@ import * as os from 'os'; import * as path from 'path'; import * as fs from 'fs-extra'; import * as nock from 'nock'; +import * as logging from '../lib/logging'; import { CachedDataSource, filterNotices, @@ -12,7 +13,6 @@ import { WebsiteNoticeDataSource, } from '../lib/notices'; import * as version from '../lib/version'; -import * as logging from '../lib/logging'; const BASIC_NOTICE = { title: 'Toggling off auto_delete_objects for Bucket empties the bucket', @@ -237,7 +237,7 @@ describe('cli notices', () => { test('returns empty array when the connection stays idle for too long', async () => { nock('https://cli.cdk.dev-tools.aws.dev') .get('/notices.json') - .delayConnection(2000) + .delayConnection(3500) .reply(200, { notices: [BASIC_NOTICE], }); @@ -250,7 +250,7 @@ describe('cli notices', () => { test('returns empty array when the request takes too long to finish', async () => { nock('https://cli.cdk.dev-tools.aws.dev') .get('/notices.json') - .delayBody(2000) + .delayBody(3500) .reply(200, { notices: [BASIC_NOTICE], });