Skip to content
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

history.Sync: Don't operate on closed channel #725

Merged
merged 1 commit into from Apr 2, 2024

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Mar 28, 2024

Before

diff --git a/pkg/icingadb/history/sync.go b/pkg/icingadb/history/sync.go
index 4be0e71..fb31d31 100644
--- a/pkg/icingadb/history/sync.go
+++ b/pkg/icingadb/history/sync.go
@@ -140,7 +140,9 @@ func (s Sync) deleteFromRedis(ctx context.Context, key string, input <-chan redi
                }
        }).Stop()
 
-       bulks := com.Bulk(ctx, input, s.redis.Options.HScanCount, com.NeverSplit[redis.XMessage])
+       justKidding := make(chan redis.XMessage)
+       close(justKidding)
+       bulks := com.Bulk(ctx, justKidding, s.redis.Options.HScanCount, com.NeverSplit[redis.XMessage])
        stream := "icinga:history:stream:" + key
        for {
                select {

2024-03-28T15:00:22.430+0100    FATAL   main    ERR wrong number of arguments for 'xdel' command
can't perform "[xdel icinga:history:stream:flapping]"
github.com/icinga/icingadb/pkg/icingaredis.WrapCmdErr
        /Users/yhabteab/Workspace/go/icingadb/pkg/icingaredis/utils.go:121
github.com/icinga/icingadb/pkg/icingadb/history.Sync.deleteFromRedis
        /Users/yhabteab/Workspace/go/icingadb/pkg/icingadb/history/sync.go:157
github.com/icinga/icingadb/pkg/icingadb/history.Sync.Sync.func3
        /Users/yhabteab/Workspace/go/icingadb/pkg/icingadb/history/sync.go:96
golang.org/x/sync/errgroup.(*Group).Go.func1
        /Users/yhabteab/go/pkg/mod/golang.org/x/sync@v0.6.0/errgroup/errgroup.go:78
runtime.goexit
        /opt/homebrew/Cellar/go/1.22.1/libexec/src/runtime/asm_arm64.s:1222
exit status 1

After

No crash

fixes #713

@cla-bot cla-bot bot added the cla/signed label Mar 28, 2024
@yhabteab yhabteab added bug Something isn't working area/history labels Mar 28, 2024
@yhabteab yhabteab added this to the 1.1.2 milestone Mar 28, 2024
@yhabteab yhabteab requested review from Al2Klimov and oxzi March 28, 2024 14:04
@julianbrost
Copy link
Contributor

julianbrost commented Mar 28, 2024

Have you checked that closing the channel is the only situation how len(bulk) == 0 could happen, i.e. that there a no nil or zero-length slices actually sent to the channel?

@yhabteab
Copy link
Member Author

i.e. that there a no nil or zero-length slices actually sent to the channel?

This is never going to happen!

icingadb/pkg/com/bulker.go

Lines 114 to 117 in 51e5434

if len(buf) > 0 {
b.ch <- buf
buf = make([]T, 0, count)
}

icingadb/pkg/com/bulker.go

Lines 130 to 132 in 51e5434

if len(buf) > 0 {
b.ch <- buf
}

@yhabteab
Copy link
Member Author

In case redis.Options.HScanCount is set to <= 1:

icingadb/pkg/com/bulker.go

Lines 165 to 171 in 51e5434

case item, ok := <-ch:
if !ok {
return
}
select {
case out <- []T{item}:

@julianbrost julianbrost merged commit 2826af4 into main Apr 2, 2024
31 checks passed
@julianbrost julianbrost deleted the wrong-number-xdel-args branch April 2, 2024 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/history bug Something isn't working cla/signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

History sync: wrong number of arguments for 'xdel' command
3 participants