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

Setting a timeout of 3 seconds for both the connection and the response, making the CLI give up fast in case of network issues or high latency. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Feb 28, 2022 · 6595d04 · 6595d04
1 parent 7300f2e
commit 6595d04
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 30 deletions.
diff --git 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<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([]);
@@ -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;
     }
   }
 

diff --git 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,
@@ -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')
@@ -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 () => {