Skip to content

Conversation

@Elia-Renzoni
Copy link
Contributor

This PR updates the usage of WaitGroups to align with Go 1.25.

Changes

  • Update WaitGroup usage to the Go 1.25 API.
  • Remove closures that previously captured loop/index variables.
  • Introduce helper variables to avoid index-out-of-bound errors that appeared after removing those closures.

Notes

  • The helper variables were added because the update removed closures that previously enclosed loop/index variables; without these helpers the code produced index-out-of-bound errors.
  • If the current helper variable names are unclear or not descriptive enough, I'm happy to rename them based on your suggestions.
  • Some parts of the updated code may still be optimized further; I welcome suggestions and feedback on possible improvements.

Related issue: #870

Thanks for reviewing — any feedback is appreciated.

@coveralls
Copy link

coveralls commented Sep 25, 2025

Pull Request Test Coverage Report for Build 19391845735

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 89 of 96 (92.71%) changed or added relevant lines in 15 files are covered.
  • 66 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.01%) to 76.2%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/backends/clickhouse/model/unmarshal.go 0 1 0.0%
pkg/timeseries/epoch/epoch.go 0 1 0.0%
pkg/proxy/engines/proxy_request.go 7 9 77.78%
pkg/timeseries/dataset/series_list.go 4 7 57.14%
Files with Coverage Reduction New Missed Lines %
pkg/proxy/engines/deltaproxycache.go 2 83.52%
pkg/cache/index/client.go 64 72.18%
Totals Coverage Status
Change from base Build 17437024522: 0.01%
Covered Lines: 15285
Relevant Lines: 20059

💛 - Coveralls

@Elia-Renzoni
Copy link
Contributor Author

@jranson I was wondering if it might be possible to merge it, so I can work on some new changes without running into conflicts.
Please let me know if there’s anything else I should adjust.
Thanks a lot 🙏

Copy link
Member

@jranson jranson left a comment

Choose a reason for hiding this comment

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

@Elia-Renzoni thanks for this, and sorry I needed more time to review it. As of go 1.22, for loop variables are properly assigned per-iteration, so they should be concurrency-safe when passed into goroutines. See notes below for when you can do that without needing any loop helper vars. The index-out-of-bounds errors are specific to variables that are not controlled by / declared in the for clause, so those non-clause helpers were OK to keep but must be declared before/outside of the wg.Go() to avoid data race conditions.

@jranson jranson merged commit 6c31fd0 into trickstercache:main Nov 15, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants