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): old setInterval remains and is not cleared #32985

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -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)));

Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like the actual fix!


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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, but I'd rather see this thrown an exception saying that stop() should have been called first.

The problem is not that start() doesn't do what it should, the problem is that the consumer is calling this class incorrectly, but it's not obvious that it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense.
f7726d9

this.setInterval = setInterval(() => {
if (!this.isPaused) {
this.print();
Expand Down
30 changes: 30 additions & 0 deletions packages/aws-cdk/test/api/garbage-collection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
Loading