Check for too big option.PipelineMultiplex value

[atercattus]

If we use ClientOption.PipelineMultiplex in the wrong way (too big), we will get a panic inside rueidis:

https://go.dev/play/p/QE8VYHE6hJX

package main

func foo(v int) int {
	return 1 << v
}

func main() {
	println(foo(0), foo(2), foo(50), foo(70), foo(100), foo(200))
}

>> 1 4 1125899906842624 0 0 0

In this case, we will get zero for multiplex value and zero-sized wire, mu, sc slices. Which will lead to panic here for example.

goroutine 59 [running]:
github.com/redis/rueidis.(*mux)._pipe(0x0?, 0x0?)
        /home/X/go/pkg/mod/github.com/redis/[email protected]/mux.go:142 +0x71e
github.com/redis/rueidis.(*mux).Dial(0x0?)
        /home/X/go/pkg/mod/github.com/redis/[email protected]/mux.go:188 +0x1b
github.com/redis/rueidis.(*clusterClient).init.func1({0xc00010f640, 0x19}, {0x17ce2d0, 0xc00001b180})
        /home/X/go/pkg/mod/github.com/redis/[email protected]/cluster.go:134 +0x48
created by github.com/redis/rueidis.(*clusterClient).init
        /home/X/go/pkg/mod/github.com/redis/[email protected]/cluster.go:133 +0x9d

[bald2b]

Is this good way? Because we just replace wrong value with default, but user never knows about problem.
I think better panic with explanation why

[rueian]

Thanks @atercattus! And I agree with @bald2b. It will be better to return an error to users.

Actually, I don't think setting PipelineMultiplex to a value larger than 2 will help. The Redis documentation also suggests that "Using more than 8 threads is unlikely to help much".

[codecov-commenter]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a85dd76) 95.96% compared to head (3f462ea) 95.96%.

Files	Patch %	Lines
rueidis.go	0.00%	2 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
- Coverage   95.96%   95.96%   -0.01%     
==========================================
  Files          77       77              
  Lines       32410    32413       +3     
==========================================
+ Hits        31103    31104       +1     
- Misses       1112     1115       +3     
+ Partials      195      194       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[atercattus]

We set too big a value by mistake and got unexpected panic :)

I think, we have two ways:

• change a signature for newMux() to newMux(...) (*mux, error) and for all functions that call it
• update docs for a given behavior.

Now I'm leaning towards the second option. What do you think?

[rueian]

We set too big a value by mistake and got unexpected panic :)

I think, we have two ways:

• change a signature for newMux() to newMux(...) (*mux, error) and for all functions that call it
• update docs for a given behavior.

Now I'm leaning towards the second option. What do you think?

I see, but changing the signature of newMux is too much. It requires too many changes.

I think we can just validate the value of PipelineMultiplex in the NewClient.

[rueian]

We can put a hard limit of 8, resulting in 256 connections to a node.

[atercattus]

Could you make a new tag? :)

[rueian]

Sure. I will do it later.