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.

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

Merged
merged 1 commit into from
Jul 15, 2025

Conversation

ldez
Copy link
Member

@ldez ldez commented Jul 15, 2025

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

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.

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

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

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

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic: close of closed channel
3 participants