`panic` when we try to get the redis result as some specific datatype

[smit245]

I am looking into this library to use it in one of my projects and i found that when we do client.Do operation and it returns RedisResult and when we use redisResult.AsBool() this return bool value of the result or error but when i looked into the method i found that when result is not actually bool the program will panic as i found at message.go#615.

I found this and i found it is a bad practice. The method is already sending the error so we can easily return error why panic? Is there any reason?

I also looked at the Command Response Cheatsheet but still, mistakes happen and for that program should not panic instead error should be returned and it will be handled by the library user and then he will find out that response of redis is not in the data type they wished for.

Please answer if it's a valid question

Otherwise i found this package amazing and loved the command builder you guys have provided also looking into other features

[rueian]

Hi @smit245,

As you mentioned, using functions like AsBool() to parse a mismatched response data type is a programming mistake. That is exactly the reason why we make it panic instead of returning an error.

[creker]

I was also burned by this multiple times. Often, this happens in DoMulti calls where the order of the results is important. Testing doesn't help much because mocked rueidis is controlled by the programmer. It might return wrong result types that happen to match the expectations of the code you're testing.

I think this should be changed to returning an error. It is a programming mistake, but it happens at runtime and causes outages in the worst case if panic is not recovered. At least with errors it allows code to continue to run and defers the problem to metrics/logs to notify the user about buggy behaviour.

And it's simply not idiomatic. Library code should panic only in very rare cases. It makes sense to panic in constructors to terminate the application before it even launches. Standard library even does this and it's ok usually. But panicking later is bad practice in almost all cases.

[rueian]

Hi @creker,

Often, this happens in DoMulti calls where the order of the results is important.

Could you provide more details on this? I thought the order of commands should not affect their response types.

Testing doesn't help much because mocked rueidis is controlled by the programmer.

That's true currently. So, I think we now have a chance to improve the rueidis mock by bounding commands with their response types in the mocked implementation, so that you will be more confident with code tested with mock.

In general, I would recommend testing with a real redis, while rueidis mock is still useful when it is hard for you to do so for any reason.

At least with errors it allows code to continue to run and defers the problem to metrics/logs to notify the user about buggy behavior. But panicking later is bad practice in almost all cases.

Not everyone agrees on this. Someone who prefers fail-fast will claim deferring a programming mistake will often lead to an even larger problem.

Yet, I understand that some people don't like this panic behavior. I also think It would be nice if the behavior is configurable.

[kevinxmorales]

I've also run into issues with panics during development, where I wasn't sure what type method to use on RedisResult values. I do think the panic's shouldn't be too bad of a problem once you figure it out in development and I personally wouldn't rely on this proposal. I was thinking making this behavior configurable could be done by adding a command before the final type method Something like this Add a noPanic boolean that determines if it should not panic (false by default, keep current behavior)

type RedisResult struct {
	err error
	val RedisMessage

	noPanic bool
}

add a NP (No Panic) method to RedisResult that sets the noPanic value to true

func (r RedisResult) NP() RedisResult {
	r.noPanic = true
	return r
}

a handle panic method that can be put into any RedisResult method that panics

func (r RedisResult) handlePanic(err error) error {
	if e := recover(); e != nil {
		err = e.(error)
	}
	return err
}

Example of its potential usage in ToArray

// ToArray delegates to RedisMessage.ToArray
func (r RedisResult) ToArray() (v []RedisMessage, err error) {
	if r.err != nil {
		err = r.err
	} else {
		v, err = r.val.ToArray()
	}
	if r.noPanic {
		err = r.handlePanic(err)
	}
	return
}

And then finally, in application code we can do this. This will ensure the error is recovered from the panic and returned in the err value

values, err := c.Do(ctx, c.B().Mget().Key("k1", "k2").Build()).NP().ToArray()

Does this seem like a workable solution to prevent panics while not having to break existing behavior with current users? It would be opt-in for anyone that wants to use the no panic behavior

[rueian]

Hi @kevinxmorales, thank you for the proposal. But I think it will be better to use a global environment variable to toggle the behavior instead, since people may not want to change their code and wait for recompilation when it panics on production because of this.

[smit245]

@rueian I have an option, If you have seen some packages of golang like entgo they have provided methods with X suffix which does not return error instead panics. Like if the meth is Get() then the panicking verson of method is GetX().

[rueian]

To summarize, we have three proposals now:

1. client.Do(ctx, cmd).NP().AsBool()
2. client.Do(ctx, cmd).AsBoolNP()
3. An implicit global env toggle

In addition, we can also enhance the rueidis mock to assert return types according to the cmds passed in.

[creker]

What about just adding new option to ClientOption? Something like NoPanics. Ideally, no panics should be the default but that would be backwards incompatible.

[rueian]

What about just adding new option to ClientOption? Something like NoPanics. Ideally, no panics should be the default but that would be backwards incompatible.

I thought about that too, but I realized that it also requires code changes to toggle the behavior if users know nothing about the option before being bitten.

[smit245]

To summarize, we have three proposals now:

1. client.Do(ctx, cmd).NP().AsBool()
2. client.Do(ctx, cmd).AsBoolNP()
3. An implicit global env toggle

In addition, we can also enhance the rueidis mock to assert return types according to the cmds passed in.

The 1st one is good because it is backward compatible and easily usable. Instead of NP() use NonPanic() so that everyone understands Otherwise it's upto you

[knadh]

.AsBool() .MustBool(), .AsInt() .MustInt() etc. Prefixing Must* for asserting and panic'ing is a convention I've seen in a few projects. Probably a pattern worth considering. Sounds a bit better than NP, NonPanic.

• https://pkg.go.dev/github.com/jmoiron/sqlx#DB.MustBegin
• https://pkg.go.dev/github.com/spf13/viper#MustBindEnv
• I follow it here myself: https://github.com/knadh/koanf/blob/master/getters.go#L118.

[rueian]

Hi @knadh,

Prefixing Must* is a good convention for removing the error from the function signature entirely.

func MustBool() bool

However, we still need errors to be returned here, such as redis errors and IO errors. Having a function with a Must* prefix but still returning an error may look weird:

func MustBool() (bool, error)

[SoulPancake]

Hi @rueian, Did we end up deciding on the proposal here?

Also, Nice to see you here @knadh :) big fan

[rueian]

After considering on this for several months, I now tend to adapt the original proposal: return an error instead of panic.

The proposal indeed simplifies everything.

[rueian]

Solved by #555