Skip to content

Commit a2166e2

Browse files
authored
fix(cli): old setInterval remains and is not cleared in garbage collection (#32985)
### Issue # (if applicable) Related to #32742 ### Reason for this change In the `garbageCollectS3` and `garbageCollectEcr` functions, a new `setInterval` is created in the `ProgressPrinter` class each time the for-await loop runs. As a result, the old `setInterval` can no longer be referenced via `this.setInterval`, and because it cannot be cleared, the messages continue to be displayed. https://github.com/aws/aws-cdk/blob/899965d6147829b8f8ac52ac8c1350de50d7b6d0/packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts#L44 This behavior is the same as what occurs in the simple sample code below. ```ts const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) class Printer { private setInterval?: ReturnType<typeof setTimeout> public start() { this.setInterval = setInterval(() => { console.log("Hello, world!") }, 1000) } public stop() { clearInterval(this.setInterval) } } ;(async () => { const printer = new Printer() // print one message per second for 5 seconds printer.start() await sleep(5000) // print tow messages per second for 5 seconds printer.start() await sleep(5000) // clear the newer setInterval // the older one will still be running printer.stop() })() ``` ### Description of changes It didn't seem necessary to initialize `setInterval` each time the for-await loop runs, so I modified the code to call `.start()` before the loop begins. ### Describe any new or updated permissions being added Nothing. ### Description of how you validated changes ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 00430ac commit a2166e2

File tree

3 files changed

+40
-2
lines changed

3 files changed

+40
-2
lines changed

packages/aws-cdk/lib/api/garbage-collection/garbage-collector.ts

+4-2
Original file line numberDiff line numberDiff line change
@@ -251,9 +251,10 @@ export class GarbageCollector {
251251

252252
debug(`Parsing through ${numImages} images in batches`);
253253

254+
printer.start();
255+
254256
for await (const batch of this.readRepoInBatches(ecr, repo, batchSize, currentTime)) {
255257
await backgroundStackRefresh.noOlderThan(600_000); // 10 mins
256-
printer.start();
257258

258259
const { included: isolated, excluded: notIsolated } = partition(batch, asset => !asset.tags.some(t => activeAssets.contains(t)));
259260

@@ -323,12 +324,13 @@ export class GarbageCollector {
323324

324325
debug(`Parsing through ${numObjects} objects in batches`);
325326

327+
printer.start();
328+
326329
// Process objects in batches of 1000
327330
// This is the batch limit of s3.DeleteObject and we intend to optimize for the "worst case" scenario
328331
// where gc is run for the first time on a long-standing bucket where ~100% of objects are isolated.
329332
for await (const batch of this.readBucketInBatches(s3, bucket, batchSize, currentTime)) {
330333
await backgroundStackRefresh.noOlderThan(600_000); // 10 mins
331-
printer.start();
332334

333335
const { included: isolated, excluded: notIsolated } = partition(batch, asset => !activeAssets.contains(asset.fileName()));
334336

packages/aws-cdk/lib/api/garbage-collection/progress-printer.ts

+8
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import * as chalk from 'chalk';
22
import { GcAsset as GCAsset } from './garbage-collector';
33
import { info } from '../../logging';
4+
import { ToolkitError } from '../../toolkit/error';
45

56
export class ProgressPrinter {
67
private totalAssets: number;
@@ -41,6 +42,13 @@ export class ProgressPrinter {
4142
}
4243

4344
public start() {
45+
// If there is already a running setInterval, throw an error.
46+
// This is because if this.setInterval is reassigned to another setInterval,
47+
// the original setInterval remains and can no longer be cleared.
48+
if (this.setInterval) {
49+
throw new ToolkitError('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');
50+
}
51+
4452
this.setInterval = setInterval(() => {
4553
if (!this.isPaused) {
4654
this.print();

packages/aws-cdk/test/api/garbage-collection.test.ts

+28
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
BackgroundStackRefresh,
2121
BackgroundStackRefreshProps,
2222
} from '../../lib/api/garbage-collection/stack-refresh';
23+
import { ProgressPrinter } from '../../lib/api/garbage-collection/progress-printer';
2324
import {
2425
BatchDeleteImageCommand,
2526
BatchGetImageCommand,
@@ -1002,6 +1003,33 @@ describe('BackgroundStackRefresh', () => {
10021003
});
10031004
});
10041005

1006+
describe('ProgressPrinter', () => {
1007+
let progressPrinter: ProgressPrinter;
1008+
let setInterval: jest.SpyInstance;
1009+
let clearInterval: jest.SpyInstance;
1010+
1011+
beforeEach(() => {
1012+
jest.useFakeTimers();
1013+
setInterval = jest.spyOn(global, 'setInterval');
1014+
clearInterval = jest.spyOn(global, 'clearInterval');
1015+
1016+
progressPrinter = new ProgressPrinter(0, 1000);
1017+
});
1018+
1019+
afterEach(() => {
1020+
jest.clearAllTimers();
1021+
setInterval.mockRestore();
1022+
clearInterval.mockRestore();
1023+
});
1024+
1025+
test('throws if start is called twice without stop', () => {
1026+
progressPrinter.start();
1027+
1028+
expect(setInterval).toHaveBeenCalledTimes(1);
1029+
expect(() => progressPrinter.start()).toThrow('ProgressPrinter is already running. Stop it first using the stop() method before starting it again.');
1030+
});
1031+
});
1032+
10051033
function daysInThePast(days: number): Date {
10061034
const d = new Date();
10071035
d.setDate(d.getDate() - days);

0 commit comments

Comments
 (0)