Handling edge cases on connSet(Read/Write)Handler

[madolson]

There are quite a few places where we call connSetReadHandler and connSetWriteHandler and don't check the return code.

From an API perspective, we only need to check the return code when we're setting the value to a non-null value, which is the only place with known failure modes. The only known failure mode I'm aware of is when you try to set a connection handler to a connection whose fd is greater than the maxclients, which is possible if the ulimit for fds is set greater than the maxclients.

When we fail to check the return code, we might end up in a situation where a connection is not registered and becomes unreachable outside of an operation like client kill. I've seen this at AWS with the primary/replica link, which can lead to a broken link.

I think we have two options:

1. Make sure we are better handling these failure modes, as in we always handle the error code when setting the read or write handler.
2. Make sure there are no failure modes on the connSetHandler path, by making sure it's a valid connection during connAccept or connConnect. connSetReadHandler and connSetWriteHandler should be changed to a void return type, and panic when they fail. (My preference)

[madolson]

I suppose another option would be to allow specifying an error handler in the API, which get's called if the API fails. That would allow us to better support the error case.

@PingXie Just want to get your opinion on this. I think we should do three things:

1. Change connSetHandler so it returns a void.
2. Attempt to handle the case where an fd is out of bounds by doing an in-place realloc of the array and log a message.
3. Call the error handler when we encounter an error.

[PingXie]

Both 1) and 2) make sense to me. Can you elaborator on 3)? I am not sure which error handler this is. If we can realloc, there isn't much for the caller to handle anyway; but if we can't, I think we have a bigger issue.

[madolson]

but if we can't, I think we have a bigger issue.

I guess I just don't know if we actually have bigger issues. Today we might just be silently ignoring issues on error paths which get resolved later. I can imagine there are cases where a client was accepted, has already been closed by the file system by some later time we install the handler, and we gracefully ignore the epoll_ctl command since the client will get eventually closed anyways.

So, my thought for the error handler, is just to make sure we aren't "leaking" the client resource (for the normal client case) and the clusterbus (in the cluster case). If after some time in product we don't observe any issue, we could remove it.

[PingXie]

If we do 1) and 2), we will be able to handle all errors internally to aeApiAddEvent so we probably don't need an error handler callback. Should we consider a fix in Valkey 8.0?