Skip to content

Conversation

@miparnisari
Copy link
Contributor

@miparnisari miparnisari commented Sep 10, 2025

This

defer goleak.VerifyNone(t, goleak.IgnoreCurrent())

and this

defer func() {
    goleak.VerifyNone(t, goleak.IgnoreCurrent())
}()

aren't the same. 1st snippet is correct, 2nd will cause goleak.IgnoreCurrent() to run at "the wrong time" and therefore mark all goroutines as ignored.

See uber-go/goleak#137

@github-actions github-actions bot added area/api v1 Affects the v1 API area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Sep 10, 2025
@codecov
Copy link

codecov bot commented Sep 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.69%. Comparing base (3af8bd0) to head (a33808b).
⚠️ Report is 30 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2557      +/-   ##
==========================================
+ Coverage   77.67%   77.69%   +0.02%     
==========================================
  Files         439      439              
  Lines       53526    53526              
==========================================
+ Hits        41573    41580       +7     
+ Misses       9358     9352       -6     
+ Partials     2595     2594       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@miparnisari miparnisari force-pushed the correct-usage-of-goleak-ignorecurrent branch from 673c807 to a33808b Compare September 10, 2025 23:14
case <-time.After(30 * time.Second):
require.Fail(t, "ungraceful server termination")
}
goleak.VerifyNone(t, goleak.IgnoreCurrent())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't do anything at the moment

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it verify it at that point?

Copy link
Contributor Author

@miparnisari miparnisari Sep 10, 2025

Choose a reason for hiding this comment

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

yes but goleak.IgnoreCurrent will blindly mark every goroutine running as ignored 😓 therefore, no leaks will be found

@miparnisari miparnisari marked this pull request as ready for review September 10, 2025 23:30
@miparnisari miparnisari requested a review from a team as a code owner September 10, 2025 23:30

func TestCertRotation(t *testing.T) {
t.Parallel()
defer goleak.VerifyNone(t, goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #2558

Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

See comment

})
t.Cleanup(cleanup)
defer goleak.VerifyNone(t, goleak.IgnoreCurrent())
defer cleanup()
Copy link
Contributor

Choose a reason for hiding this comment

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

The above change makes sense to me, but why did this get changed from Cleanup to defer?

Copy link
Contributor Author

@miparnisari miparnisari Sep 17, 2025

Choose a reason for hiding this comment

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

if I could do t.Cleanup(goleak.VerifyNone...) then i think that would be equivalent to what I did, but i can't do that because t.Cleanup expects a function signature that goleak can't provide

Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

LGTM

@miparnisari miparnisari added this pull request to the merge queue Sep 17, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 17, 2025
@miparnisari miparnisari added this pull request to the merge queue Sep 17, 2025
Merged via the queue into main with commit 148dca7 Sep 17, 2025
46 checks passed
@miparnisari miparnisari deleted the correct-usage-of-goleak-ignorecurrent branch September 17, 2025 17:11
@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area/api v1 Affects the v1 API area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants