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 4 commits into
base: main
Choose a base branch
from

Conversation

sakurai-ryo
Copy link
Contributor

@sakurai-ryo sakurai-ryo commented Jan 17, 2025

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.

this.setInterval = setInterval(() => {

This behavior is the same as what occurs in the simple sample code below.

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


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the p2 label Jan 17, 2025
@aws-cdk-automation aws-cdk-automation requested a review from a team January 17, 2025 09:02
@github-actions github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Jan 17, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/32985/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jan 17, 2025
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.48%. Comparing base (2b2443d) to head (19ae427).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #32985   +/-   ##
=======================================
  Coverage   81.48%   81.48%           
=======================================
  Files         226      226           
  Lines       13768    13770    +2     
  Branches     2416     2417    +1     
=======================================
+ Hits        11219    11221    +2     
  Misses       2271     2271           
  Partials      278      278           
Flag Coverage Δ
suite.unit 81.48% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk 80.90% <100.00%> (+<0.01%) ⬆️
packages/aws-cdk-lib/core 82.10% <ø> (ø)

Comment on lines 44 to 50
// 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

// 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!

@rix0rrr rix0rrr added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jan 17, 2025
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/32985/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

@sakurai-ryo sakurai-ryo marked this pull request as ready for review January 17, 2025 10:42
@sakurai-ryo sakurai-ryo requested a review from a team as a code owner January 17, 2025 10:42
@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: fa26047
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rix0rrr rix0rrr added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Jan 17, 2025
@aws-cdk-automation aws-cdk-automation dismissed their stale review January 17, 2025 14:08

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Copy link
Contributor

mergify bot commented Jan 17, 2025

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@kaizencc
Copy link
Contributor

@Mergifyio update

Copy link
Contributor

mergify bot commented Jan 17, 2025

update

❌ Mergify doesn't have permission to update

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/check-suite-test.yaml without workflows permission

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 pr/needs-cli-test-run This PR needs CLI tests run against it. pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/exempt-integ-test The PR linter will not require integ test changes star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants