Skip to content

fix: panic: close of closed channel #5939

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.

Sign up for GitHub

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

Jump to bottom
Merged
ldez merged 1 commit into from
Jul 15, 2025
Merged

fix: panic: close of closed channel #5939

ldez merged 1 commit into from
Jul 15, 2025

Conversation

@ldez
Copy link
Member

@ldez ldez commented Jul 15, 2025 •
edited
Loading

I used a global cancellable context because cancel is concurrency safe (and so can be called several times).

The background of the problem is the fact of having go routines inside go routines inside go routines.

I think I will rewrite this section at some point.

To reproduce, I replaced Next with a non-existent method Foo.

git clone https://github.com/NVIDIA/aistore.git
cd aistore
git checkout 7730e4290
diff --git i/cmd/cli/cli/lhotse.go w/cmd/cli/cli/lhotse.go
index 1a13987aa..c16e0b1a2 100644
--- i/cmd/cli/cli/lhotse.go
+++ w/cmd/cli/cli/lhotse.go
@@ -262,7 +262,7 @@ func lhotseMultiBatch(c *cli.Context, outCtx *mossReqParseCtx) error {
                // when batch is full
                if len(currentBatch) >= outCtx.batchSize {
                        // Get next filename from template
-                       shardName, hasNext := outCtx.pt.Next()
+                       shardName, hasNext := outCtx.pt.Foo()
                        if !hasNext {
                                return fmt.Errorf("template exhausted at batch %d (generated %d batches so far)", totalBatches+1, totalBatches)
                        }

Fixes #5938
Related to #5929, #5923

@ldez ldez added the bug Something isn't working label Jul 15, 2025
@ldez ldez added this to the v2-unreleased milestone Jul 15, 2025
@alex-aizman
Copy link

alex-aizman commented Jul 15, 2025

for stopChan, maybe consider this type pattern:

@ldez
Copy link
Member Author

ldez commented Jul 15, 2025

The usage of context in this situation seems better because of at least the parent-child relationship.

bombsimon
bombsimon approved these changes Jul 15, 2025
View reviewed changes
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/goanalysis/runner_loadingpackage.go
Comment on lines +71 to +75
select {
case <-ctx.Done():
return
default:
}
Copy link
Member

@bombsimon bombsimon Jul 15, 2025

Choose a reason for hiding this comment

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

It should be logically the same but when ctx.Done() is closed ctx.Err() will return a non nil error. Just a personal pref on what's easier to read I guess.

Suggested change
select {
case <-ctx.Done():
return
default:
}
if ctx.Err() != nil {
return
}

Copy link
Member Author

@ldez ldez Jul 15, 2025

Choose a reason for hiding this comment

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

In this situation, for me, checking the error is confusing because we don't expect an error but a stop, and it will act like we are ignoring an error.

It's a personal pref too, but in this context, I prefer using Done.

Copy link
Member

@bombsimon bombsimon Jul 15, 2025

Choose a reason for hiding this comment

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

Go for it, either works. ✅

@ldez ldez merged commit 9298bc0 into golangci:main Jul 15, 2025
18 checks passed
@ldez ldez deleted the fix/panic-closed-channel branch July 15, 2025 14:37
@ldez ldez modified the milestones: v2-unreleased, v2.3 Jul 21, 2025
alaudaa-renovate bot added a commit to AlaudaDevops/golangci-lint that referenced this pull request Aug 5, 2025
@alaudaa-renovate @dependabot
@ldez @golangci-releaser @alexandear @mgabeler-lee-6rs @kasaikou @G-Rath @kyu08 @logica0419 @manuelarte @chris-reeves @nickfromXXII @sivchari @lvlcn-t @cooperbenson-qz @Crocmagnon @take0fit @bombsimon @ccoVeille @AlwxSin @scop
chore(deps): update dependency go to v1.24.5 (alauda-v1.64.8) (#6)
207f29f 
* build(deps): bump github.com/go-critic/go-critic from 0.12.0 to 0.13.0 (golangci#5579)

* build(deps): bump mvdan.cc/unparam to HEAD (golangci#5584)

* build(deps): bump github.com/timakin/bodyclose from ed6a65f985e3 to 1db5c5ca4d67 (golangci#5585)

* docs: improve Docker example with cache reuse (golangci#5586)

* docs: add a migration guide for VSCode integration (golangci#5587)

* build(deps): bump github.com/ckaznocha/intrange from 0.3.0 to 0.3.1 (golangci#5589)

* feat: add option stdin for fmt command (golangci#5588)

* build(deps): bump github.com/daixiang0/gci from 0.13.5 to 0.13.6 (golangci#5592)

* fix: funlen ignore-comments (golangci#5594)

* docs: cleaning

* chore: prepare release

* docs: update GitHub Action assets (golangci#5595)

* docs: fix displaying GoLand logo on a dark theme (golangci#5598)

* fix: validate version before configuration (golangci#5599)

* fix: copy golines settings during linter settings load (golangci#5607)

* fix: forbidigo migration (golangci#5606)

* docs: add example of the field version (golangci#5609)

* chore: prepare release

* docs: update GitHub Action assets (golangci#5611)

Co-authored-by: Fernandez Ludovic <