From 2a31e2b1928bb6e72ca56097b1d864915b47ac50 Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Fri, 17 Jan 2025 17:56:53 +0900 Subject: [PATCH 1/4] fix(cli): old setInterval remains and is not cleared --- .../lib/api/garbage-collection/garbage-collector.ts | 6 ++++-- .../aws-cdk/lib/api/garbage-collection/progress-printer.ts | 7 +++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/lib/api/garbage-collection/garbage-collector.ts b/packages/aws-cdk/lib/api/garbage-collection/garbage-collector.ts index 5869071d4a6cd..e006522797a8c 100644 --- a/packages/aws-cdk/lib/api/garbage-collection/garbage-collector.ts +++ b/packages/aws-cdk/lib/api/garbage-collection/garbage-collector.ts @@ -251,9 +251,10 @@ export class GarbageCollector { debug(`Parsing through ${numImages} images in batches`); + printer.start(); + for await (const batch of this.readRepoInBatches(ecr, repo, batchSize, currentTime)) { await backgroundStackRefresh.noOlderThan(600_000); // 10 mins - printer.start(); const { included: isolated, excluded: notIsolated } = partition(batch, asset => !asset.tags.some(t => activeAssets.contains(t))); @@ -323,12 +324,13 @@ export class GarbageCollector { debug(`Parsing through ${numObjects} objects in batches`); + printer.start(); + // Process objects in batches of 1000 // This is the batch limit of s3.DeleteObject and we intend to optimize for the "worst case" scenario // where gc is run for the first time on a long-standing bucket where ~100% of objects are isolated. for await (const batch of this.readBucketInBatches(s3, bucket, batchSize, currentTime)) { await backgroundStackRefresh.noOlderThan(600_000); // 10 mins - printer.start(); const { included: isolated, excluded: notIsolated } = partition(batch, asset => !activeAssets.contains(asset.fileName())); diff --git a/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts b/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts index 9790dadeb0c5a..ab3a3cd9bbf3f 100644 --- a/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts +++ b/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts @@ -41,6 +41,13 @@ export class ProgressPrinter { } public start() { + // If there is already a running setInterval, clear it first. + // This is because if this.setInterval is reassigned to another setInterval, + // the original setInterval remains and can no longer be cleared. + if (this.setInterval) { + clearInterval(this.setInterval); + } + this.setInterval = setInterval(() => { if (!this.isPaused) { this.print(); From 19ae4279c7bede6095e3932d5dd420df5cbb509d Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Fri, 17 Jan 2025 18:46:40 +0900 Subject: [PATCH 2/4] add test --- .../test/api/garbage-collection.test.ts | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/packages/aws-cdk/test/api/garbage-collection.test.ts b/packages/aws-cdk/test/api/garbage-collection.test.ts index d6a55dcf951ff..c260150e8c502 100644 --- a/packages/aws-cdk/test/api/garbage-collection.test.ts +++ b/packages/aws-cdk/test/api/garbage-collection.test.ts @@ -20,6 +20,7 @@ import { BackgroundStackRefresh, BackgroundStackRefreshProps, } from '../../lib/api/garbage-collection/stack-refresh'; +import { ProgressPrinter } from '../../lib/api/garbage-collection/progress-printer'; import { BatchDeleteImageCommand, BatchGetImageCommand, @@ -1002,6 +1003,35 @@ describe('BackgroundStackRefresh', () => { }); }); +describe('ProgressPrinter', () => { + let progressPrinter: ProgressPrinter; + let setInterval: jest.SpyInstance; + let clearInterval: jest.SpyInstance; + + beforeEach(() => { + jest.useFakeTimers(); + setInterval = jest.spyOn(global, 'setInterval'); + clearInterval = jest.spyOn(global, 'clearInterval'); + + progressPrinter = new ProgressPrinter(0, 1000); + }); + + afterEach(() => { + jest.clearAllTimers(); + setInterval.mockRestore(); + clearInterval.mockRestore(); + }); + + test('clears the previous interval before creating a new one', () => { + progressPrinter.start(); + progressPrinter.start(); + + expect(setInterval).toHaveBeenCalledTimes(2); + // check that the older interval is cleared before creating a new one + expect(clearInterval).toHaveBeenCalledTimes(1); + }); +}); + function daysInThePast(days: number): Date { const d = new Date(); d.setDate(d.getDate() - days); From f7726d998a45b97e3c6ff7b6a8ec5dcfe4b709c8 Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Fri, 17 Jan 2025 19:41:13 +0900 Subject: [PATCH 3/4] throw an error when start called continuously --- .../lib/api/garbage-collection/progress-printer.ts | 4 ++-- packages/aws-cdk/test/api/garbage-collection.test.ts | 8 +++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts b/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts index ab3a3cd9bbf3f..0ab0fd85494eb 100644 --- a/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts +++ b/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts @@ -41,11 +41,11 @@ export class ProgressPrinter { } public start() { - // If there is already a running setInterval, clear it first. + // If there is already a running setInterval, throw an error. // This is because if this.setInterval is reassigned to another setInterval, // the original setInterval remains and can no longer be cleared. if (this.setInterval) { - clearInterval(this.setInterval); + throw new Error('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.'); } this.setInterval = setInterval(() => { diff --git a/packages/aws-cdk/test/api/garbage-collection.test.ts b/packages/aws-cdk/test/api/garbage-collection.test.ts index c260150e8c502..158b6b0f7c53d 100644 --- a/packages/aws-cdk/test/api/garbage-collection.test.ts +++ b/packages/aws-cdk/test/api/garbage-collection.test.ts @@ -1022,13 +1022,11 @@ describe('ProgressPrinter', () => { clearInterval.mockRestore(); }); - test('clears the previous interval before creating a new one', () => { - progressPrinter.start(); + test('throws if start is called twice without stop', () => { progressPrinter.start(); - expect(setInterval).toHaveBeenCalledTimes(2); - // check that the older interval is cleared before creating a new one - expect(clearInterval).toHaveBeenCalledTimes(1); + expect(setInterval).toHaveBeenCalledTimes(1); + expect(() => progressPrinter.start()).toThrow('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.'); }); }); From 70c7b85a5ad6f84202fd99fe8a86c1af343747ea Mon Sep 17 00:00:00 2001 From: sakurai-ryo Date: Fri, 31 Jan 2025 11:12:28 +0900 Subject: [PATCH 4/4] use ToolkitError --- .../aws-cdk/lib/api/garbage-collection/progress-printer.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts b/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts index 0ab0fd85494eb..bca2eb7fb7812 100644 --- a/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts +++ b/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts @@ -1,6 +1,7 @@ import * as chalk from 'chalk'; import { GcAsset as GCAsset } from './garbage-collector'; import { info } from '../../logging'; +import { ToolkitError } from '../../toolkit/error'; export class ProgressPrinter { private totalAssets: number; @@ -45,7 +46,7 @@ export class ProgressPrinter { // This is because if this.setInterval is reassigned to another setInterval, // the original setInterval remains and can no longer be cleared. if (this.setInterval) { - throw new Error('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.'); + throw new ToolkitError('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.'); } this.setInterval = setInterval(() => {