Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): long connection timeout slows the CLI down #19187

Merged
merged 3 commits into from
Feb 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 41 additions & 28 deletions packages/aws-cdk/lib/notices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,37 +105,46 @@ export interface NoticeDataSource {

export class WebsiteNoticeDataSource implements NoticeDataSource {
fetch(): Promise<Notice[]> {
const timeout = 3000;

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([]);
Expand Down Expand Up @@ -176,14 +185,18 @@ export class CachedDataSource implements NoticeDataSource {
}

private async load(): Promise<CachedNotices> {
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;
}
}

Expand Down
35 changes: 33 additions & 2 deletions packages/aws-cdk/test/notices.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(3500)
.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(3500)
.reply(200, {
notices: [BASIC_NOTICE],
});

const result = await dataSource.fetch();

expect(result).toEqual([]);
});

function mockCall(statusCode: number, body: any): Promise<Notice[]> {
nock('https://cli.cdk.dev-tools.aws.dev')
.get('/notices.json')
Expand Down Expand Up @@ -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 () => {
Expand Down