Skip to content

Close the Progress Bar Container (Resource Leak)#28961

Open
VLADO2000 wants to merge 1 commit into
podman-container-tools:mainfrom
VLADO2000:patch-1
Open

Close the Progress Bar Container (Resource Leak)#28961
VLADO2000 wants to merge 1 commit into
podman-container-tools:mainfrom
VLADO2000:patch-1

Conversation

@VLADO2000

@VLADO2000 VLADO2000 commented Jun 18, 2026

Copy link
Copy Markdown

In ProgressBar, initialization of an mpb.New(...) container, but it returns alongside the bar without waiting for it or closing it. The mpb package leaks a goroutine if p.Wait() is never called on the returned *mpb.Progress instance by the caller.

To ensure the caller knows they must wait for it, updating the comment documentation is highly recommended so they know to defer p.Wait().

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

  • Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
    commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
    the sign-off email address. See CONTRIBUTING.md
    for more information.
  • Referenced issues using Fixes: #00000 in commit message (if applicable)
  • Tests have been added/updated (or no tests are needed)
  • Documentation has been updated (or no documentation changes are needed)
  • All commits pass make validatepr (format/lint checks)
  • Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

None

In ProgressBar, initialization of an mpb.New(...) container, but it returns alongside the bar without waiting for it or closing it. The mpb package leaks a goroutine if p.Wait() is never called on the returned *mpb.Progress instance by the caller.

To ensure the caller knows they must wait for it, updating the comment documentation is highly recommended so they know to defer p.Wait().

Signed-off-by: WLOT <96411435+VLADO2000@users.noreply.github.com>

@Honny1 Honny1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, Thanks!

@Honny1 Honny1 added the No New Tests Allow PR to proceed without adding regression tests label Jun 18, 2026
@Honny1

Honny1 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

PTAL @podman-container-tools/podman-reviewers @podman-container-tools/podman-maintainers

@Honny1

Honny1 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

@VLADO2000, can you please fix formatting? Thanks.

@Luap99 Luap99 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The commit title does not match the logic?

All you do is add some doc comment but your title makes it sounds like you address a leak which is not true at all.

it should likely be more like "add doc comment to ProgressBar"

Comment thread utils/utils.go

// ProgressBar initializes a progress bar container and an individual bar.
//
// CRITICAL: The caller MUST call `p.Wait()` on the returned *mpb.Progress

@mtrmac mtrmac Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(Given that this needs an update anyway:) “critical” is overdoing it, this is a fairly ordinary part of the function’s contract. I think that prefix can be just dropped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No New Tests Allow PR to proceed without adding regression tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants