Repository: redis/go-redis

Issue:
fatal error: concurrent map iteration and map write
```
fatal error: concurrent map iteration and map write

goroutine 42 [running]:
runtime.throw({0xe66a8d?, 0xc000600000?})
	/usr/local/go/src/runtime/panic.go:992 +0x71 fp=0xc00e496ec0 sp=0xc00e496e90 pc=0x438731
runtime.mapiternext(0xc1bcb72518?)
	/usr/local/go/src/runtime/map.go:871 +0x4eb fp=0xc00e496f30 sp=0xc00e496ec0 pc=0x41002b
reflect.mapiternext(0x49ad8f?)
	/usr/local/go/src/runtime/map.go:1378 +0x19 fp=0xc00e496f48 sp=0xc00e496f30 pc=0x464ff9
reflect.(*MapIter).Next(0xc1bcb72500?)
	/usr/local/go/src/reflect/value.go:1789 +0x7e fp=0xc00e496f78 sp=0xc00e496f48 pc=0x49afbe
internal/fmtsort.Sort({0xd3a3a0?, 0xc00023a9c8?, 0xc00023a9c0?})
	/usr/local/go/src/internal/fmtsort/sort.go:63 +0x159 fp=0xc00e497038 sp=0xc00e496f78 pc=0x4a9dd9
fmt.(*pp).printValue(0xc031a2e750, {0xd3a3a0?, 0xc00023a9c8?, 0xc0113c5860?}, 0x76, 0x2)
	/usr/local/go/src/fmt/print.go:769 +0x445 fp=0xc00e497220 sp=0xc00e497038 pc=0x4e7885
fmt.(*pp).printValue(0xc031a2e750, {0xe07c40?, 0xc00023a9a0?, 0x1b9?}, 0x76, 0x1)
	/usr/local/go/src/fmt/print.go:806 +0x124e fp=0xc00e497408 sp=0xc00e497220 pc=0x4e868e
fmt.(*pp).printValue(0xc031a2e750, {0xe0ed00?, 0xc000412b00?, 0xde61b0?}, 0x76, 0x0)
	/usr/local/go/src/fmt/print.go:876 +0xce5 fp=0xc00e4975f0 sp=0xc00e497408 pc=0x4e8125
fmt.(*pp).badVerb(0xc031a2e750, 0x412b00?)
	/usr/local/go/src/fmt/print.go:343 +0x457 fp=0xc00e497698 sp=0xc00e4975f0 pc=0x4e4bd7
fmt.(*pp).fmtPointer(0xc031a2e750, {0xe0ed00?, 0xc000412b00?, 0x0?}, 0x73)
	/usr/local/go/src/fmt/print.go:532 +0x418 fp=0xc00e497738 sp=0xc00e497698 pc=0x4e5e98
fmt.(*pp).printValue(0xc031a2e750, {0xe0ed00?, 0xc000412b00?, 0x0?}, 0x73, 0x2)
	/usr/local/go/src/fmt/print.go:882 +0xef7 fp=0xc00e497920 sp=0xc00e497738 pc=0x4e8337
fmt.(*pp).printValue(0xc031a2e750, {0xde6160?, 0xc000412b00?, 0xc000471b80?}, 0x73, 0x1)
	/usr/local/go/src/fmt/print.go:806 +0x124e fp=0xc00e497b08 sp=0xc00e497920 pc=0x4e868e
fmt.(*pp).printValue(0xc031a2e750, {0xd59b40?, 0xc000412b00?, 0xc005e6b220?}, 0x73, 0x0)
	/usr/local/go/src/fmt/print.go:876 +0xce5 fp=0xc00e497cf0 sp=0xc00e497b08 pc=0x4e8125
fmt.(*pp).printArg(0xc031a2e750, {0xd59b40?, 0xc000412b00}, 0x73)
	/usr/local/go/src/fmt/print.go:712 +0x74c fp=0xc00e497d90 sp=0xc00e497cf0 pc=0x4e73ac
fmt.(*pp).doPrintf(0xc031a2e750, {0xe72c9a, 0x35}, {0xc0a1e6cc00?, 0x2, 0x2})
	/usr/local/go/src/fmt/print.go:1026 +0x288 fp=0xc00e497e88 sp=0xc00e497d90 pc=0x4e9ba8
fmt.Sprintf({0xe72c9a, 0x35}, {0xc0a1e6cc00, 0x2, 0x2})
	/usr/local/go/src/fmt/print.go:219 +0x59 fp=0xc00e497ee0 sp=0xc00e497e88 pc=0x4e4019
github.com/go-redis/redis/v8/internal.(*logger).Printf(0x1570180, {0xc000471f68?, 0xc0000cc008?}, {0xe72c9a?, 0x1?}, {0xc0a1e6cc00?, 0xc0005847b0?, 0xc000584780?})
	/root/go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/internal/log.go:19 +0x37 fp=0xc00e497f18 sp=0xc00e497ee0 pc=0x64cad7
github.com/go-redis/redis/v8.(*channel).initMsgChan.func1()
	/root/go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/pubsub.go:605 +0x31a fp=0xc00e497fe0 sp=0xc00e497f18 pc=0x69563a
runtime.goexit()
	/usr/local/go/src/runtime/asm_amd64.s:1571 +0x1 fp=0xc00e497fe8 sp=0xc00e497fe0 pc=0x46b601
created by github.com/go-redis/redis/v8.(*channel).initMsgChan
	/root/go/pkg/mod/github.com/go-redis/redis/v8@v8.11.5/pubsub.go:565 +0xca

```

The error is reported as above.

I found that in many places in the code, the struct object containing the map field will be directly passed in as a log parameter, which will trigger the iteration of the map when logging, but the map is not locked, so there is a hidden danger of triggering a panic.

``` go
internal.Logger.Printf(
	ctx, "redis: %s channel is full for %s (message is dropped)",
	c, c.chanSendTimeout)
```

Discussion:
@tzq0301: Could you please provide the code that can reproduce this bug? It doesn't need to reproduce it 100%, or even just some ideas on how to trigger this bug would be helpful. Thanks very much.@jekhy: > Could you please provide the code that can reproduce this bug? It doesn't need to reproduce it 100%, or even just some ideas on how to trigger this bug would be helpful. Thanks very much.

This is an occasional bug in a specific situation, but it will cause a panic and terminate the program.
I suggest removing the parameter 'c', because it contains the 'map' type fields.@ndyakov: @jekhy is this still present with v9 ?@ndyakov: @jekhy is this still present with v9 ?@jekhy: > [@jekhy](https://github.com/jekhy) is this still present with v9 ?

Sorry, I haven't been paying attention to this issue recently. I have avoided overloading by increasing the channel capacity, so this bug has not been triggered again.

However, I took a look at the code and found that the parameter replacing %s in the log parameter is still a complete channel structure, so the hidden danger of triggering panic still exists.

![Image](https://github.com/user-attachments/assets/ca790fdd-7b6c-45ef-a41e-e679544a7fd7)@ndyakov: Okey, marking this as `good-first-issue`. Simply removing the channel from the log will help. Also if we can map the name of the subscription to a channel and include that in the logger, this will be great. @Fungx: @ndyakov Hi! I'm new to go-redis, but I'd love to help address this issue. Could you please clarify how we should map the channel name in this case? Would we need to create individual names for each `*channel`, or is there another recommended approach?@derwesx: Well.... I came up with 2 ideas:

(Easy way) Making something like `func (c *channel) Name()` that will return something from [size, pointer, channel-type (message, sub), etc]. And just calling it instead of formatting c as %s.

Or (And)

> Also if we can map the name of the subscription to a channel and include that in the logger, this will be great.

I kind of like this idea, but I don't really get wym by "subscription." Is it the info from:
```go
channel.pubSub:
	channels  map[string]struct{}
	patterns  map[string]struct{}
	schannels map[string]struct{}
```
If yes, then I will also include them in `Name()`, however, I'm a bit concerned about locking `c.pubSub.mu` to safely access the maps. It seems a bit slow, but overall, writing the channel itself to the logs should be more time-consuming than locking, so... I guess its fine? (I don't yet know how to write benchmarks on this & how to test logs with `<-timer.C` without random tweaks; Would be grateful for any advice.)@derwesx: And I guess [locking pubsub](https://github.com/derwesx/go-redis/commit/11c079d57a4d5f25da90ad1ccd3f9d79732ddd80) during formatting channel will also fix the issue.@ndyakov: @derwesx feel free to review #3360 

Solution patch:
diff --git a/commands.go b/commands.go
index bca7d7e..2713232 100644
--- a/commands.go
+++ b/commands.go
@@ -155,6 +155,12 @@ func isEmptyValue(v reflect.Value) bool {
 		return v.Float() == 0
 	case reflect.Interface, reflect.Pointer:
 		return v.IsNil()
+	case reflect.Struct:
+		if v.Type() == reflect.TypeOf(time.Time{}) {
+			return v.IsZero()
+		}
+		// Only supports the struct time.Time,
+		// subsequent iterations will follow the func Scan support decoder.
 	}
 	return false
 }
diff --git a/pubsub.go b/pubsub.go
index 20c085f..2a0e7a8 100644
--- a/pubsub.go
+++ b/pubsub.go
@@ -45,6 +45,9 @@ func (c *PubSub) init() {
 }
 
 func (c *PubSub) String() string {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
 	channels := mapKeys(c.channels)
 	channels = append(channels, mapKeys(c.patterns)...)
 	channels = append(channels, mapKeys(c.schannels)...)



Test patch:
diff --git a/commands_test.go b/commands_test.go
index 6a76756..8b2aa37 100644
--- a/commands_test.go
+++ b/commands_test.go
@@ -2578,6 +2578,63 @@ var _ = Describe("Commands", func() {
 				"val2",
 				"val",
 			}))
+
+			type setOmitEmpty struct {
+				Set1 string        `redis:"set1"`
+				Set2 int           `redis:"set2,omitempty"`
+				Set3 time.Duration `redis:"set3,omitempty"`
+				Set4 string        `redis:"set4,omitempty"`
+				Set5 time.Time     `redis:"set5,omitempty"`
+				Set6 *numberStruct `redis:"set6,omitempty"`
+				Set7 numberStruct  `redis:"set7,omitempty"`
+			}
+
+			hSet = client.HSet(ctx, "hash3", &setOmitEmpty{
+				Set1: "val",
+			})
+			Expect(hSet.Err()).NotTo(HaveOccurred())
+			// both set1 and set7 are set
+			// custom struct is not omitted
+			Expect(hSet.Val()).To(Equal(int64(2)))
+
+			hGetAll := client.HGetAll(ctx, "hash3")
+			Expect(hGetAll.Err()).NotTo(HaveOccurred())
+			Expect(hGetAll.Val()).To(Equal(map[string]string{
+				"set1": "val",
+				"set7": `{"Number":0}`,
+			}))
+			var hash3 setOmitEmpty
+			Expect(hGetAll.Scan(&hash3)).NotTo(HaveOccurred())
+			Expect(hash3.Set1).To(Equal("val"))
+			Expect(hash3.Set2).To(Equal(0))
+			Expect(hash3.Set3).To(Equal(time.Duration(0)))
+			Expect(hash3.Set4).To(Equal(""))
+			Expect(hash3.Set5).To(Equal(time.Time{}))
+			Expect(hash3.Set6).To(BeNil())
+			Expect(hash3.Set7).To(Equal(numberStruct{}))
+
+			now := time.Now()
+			hSet = client.HSet(ctx, "hash4", setOmitEmpty{
+				Set1: "val",
+				Set5: now,
+				Set6: &numberStruct{
+					Number: 5,
+				},
+				Set7: numberStruct{
+					Number: 3,
+				},
+			})
+			Expect(hSet.Err()).NotTo(HaveOccurred())
+			Expect(hSet.Val()).To(Equal(int64(4)))
+
+			hGetAll = client.HGetAll(ctx, "hash4")
+			Expect(hGetAll.Err()).NotTo(HaveOccurred())
+			Expect(hGetAll.Val()).To(Equal(map[string]string{
+				"set1": "val",
+				"set5": now.Format(time.RFC3339Nano),
+				"set6": `{"Number":5}`,
+				"set7": `{"Number":3}`,
+			}))
 		})
 
 		It("should HSetNX", func() {
@@ -7619,12 +7676,16 @@ type numberStruct struct {
 	Number int
 }
 
-func (s *numberStruct) MarshalBinary() ([]byte, error) {
-	return json.Marshal(s)
+func (n numberStruct) MarshalBinary() ([]byte, error) {
+	return json.Marshal(n)
+}
+
+func (n *numberStruct) UnmarshalBinary(b []byte) error {
+	return json.Unmarshal(b, n)
 }
 
-func (s *numberStruct) UnmarshalBinary(b []byte) error {
-	return json.Unmarshal(b, s)
+func (n *numberStruct) ScanRedis(str string) error {
+	return json.Unmarshal([]byte(str), n)
 }
 
 func deref(viface interface{}) interface{} {