add a command_timeout to redisOptions

[valentinogeron]

the user can now specify both connect_timeout and command_timeout
when creating redis context.

[valentinogeron]

@michael-grunder - as we talked in #829, I added an option for connect_timeout and command_timeout in redisOptions so the user can specify both

[michael-grunder]

This is a nice quality of life improvement.

What do you think about a minor refactor on my branch?

It's a bit easier for me to follow but doesn't change any functionality.

[valentinogeron]

looks good!
small question: what EL stands for?

[michael-grunder]

I'm guessing it stands for "event library" but I didn't initially name them so I could be wrong.

[valentinogeron]

make sense. BTW - there are any other comments?

[michael-grunder]

No, it looks good but I'll run it through an async testing setup I've got before merging.

I think we should include it in v1.0.0

[valentinogeron]

@michael-grunder - any updates?

[michael-grunder]

I'm putting together v1.0.0 and will try to get it included.

I may tweak the change a little bit to use the existing redisContext->timeout name, with an additional member redisContext->command_timeout. It's less descriptive but it would make the changes completely backward-compatible (I think. I need to go through it in more detail to be sure).

[michael-grunder]

@yossigo Any objections to merging this for v1.0.0? Depending on how much of a stickler we want to be to semver I think even an additional struct field is technically a breaking change and would need to wait for 2.0.0

The alternative is to reserve the additional timeout field in v1.0.0 and push the feature in 1.0.1 which we would able to do while maintaining 100% ABI compatibility (and 100% source compatibility if we don't rename timeout -> connect_timeout).

I'd merge this version of the PR on my fork

[yossigo]

@michael-grunder I think it can be part of 1.0.0.

BTW I am in favor of being strict with such changes, especially as hiredis traditionally offered basically no encapsulation and all major structs are all over the header files so every little change is a potential ABI breakage.

[michael-grunder]

BTW I am in favor of being strict with such changes, especially as hiredis traditionally offered basically no encapsulation and all major structs are all over the header files so every little change is a potential ABI breakage.

Agreed. Might be worth looking at using opaque structures for the next major version although that's a long ways off.

[michael-grunder]

@valentinogeron Thanks, merged! I'm going to get this merged into the release/1.0.0-rc1 branch and update the docs and changelog.

[wbangna]

This change showed one negative effect for me.
I implemented a client that may be configured to use TLS.
When this client tries to connect to a redis port that does not use TLS, the timeout setting does work for the TCP connect but not for the SSL connect. This leads to a blocking client.

redisSSLContextError ssl_error = 0;
redis_ssl_ctx = redisCreateSSLContext(
                        ca_filename, nullptr /*capath*/, cert_filename,
                        key_filename, nullptr /*sni*/, &ssl_error);

if (!redis_ssl_ctx || ssl_error != 0)
{
    /* error handling here */
}

redis_ctx = redisConnectWithTimeout(server_name, server_port, timeout);

/* Workaround: */
/* redisContextSetTimeout(redis_ctx, timeout); */

if ( redis_ctx->err != 0 )
{
    /* error handling here */
}

if (redisInitiateSSLWithContext(redis_ctx, redis_ssl_ctx) != REDIS_OK)
{
    /* error handling here */
}

Is the workaround (commented out) the right solution or should I open an issue?

Edit:
I had to forward-declare redisContextSetTimeout() (from net.h) to be able to implement the workaround.

[valentinogeron]

@wbangna - The PR goal is to have the ability to control the connect timeout and the command timeout separately.
The TLS handshake occurs after a TCP connection is established (the connect timeout is not relevant from this part), so you must specify command_timeout as well.

Your workaround will work, but you need to use redisSetTimeout instead.
Another option is to use redisConnectWithOptions. With this API, you can configure whatever you want.
For example:

struct timeval connect_timeout = { 1, 500000 };
struct timeval command_timeout = { 2, 500000 };

redisOptions options = {0};
REDIS_OPTIONS_SET_TCP(&options, ip, port);
options.connect_timeout = &connect_timeout;
options.command_timeout = &command_timeout;
redisContext *ctx = redisConnectWithOptions(&options);

[wbangna]

@valentinogeron Thank you for your response. My test was successful with the redisConnectWithOptions() function.