hiredis blocking uninterruptible

[RP991]

I recently upgraded our hiredis client library from 0.10.1 to 1.1.0.
Following the upgrade, our processes that use the BRPOPLPUSH verb ceased
responding to SIGINT or SIGTERM, i.e. became uninterruptible while blocked
waiting for a new element on the list. The processes in question have
been running for years happily with the old driver.

I tracked this down to a change in the handling of the error return from recv()
(net.c redisNetRead() in the case of 1.1.0), versus similar handling for read()
in the old version (hiredis.c redisBufferRead() in 0.10.1).

The old version used to respond to EINTR by setting REDIS_ERR_IO with
__redisSetError(c,REDIS_ERR_IO,NULL) and return REDIS_ERR (-1).

The new version "ignores" EINTR by returning 0 from redisNetRead(), without
setting REDIS_ERR_IO. This in turn prevents redisGetReply() from ever
breaking out of the while(aux == NULL) loop.

I changed redisNetRead() to treat EINTR as it used to, by calling
__redisSetError(c, REDIS_ERR_IO, strerror(errno)); and returing -1, which
solves the problem for me.

Have I misunderstood something, or is this a bug? If so, is my fix the
correct way to rectify it?

[michael-grunder]

Thanks for the report.

It looks like this change was made very early in the project (see: #99). It appears that the consensus was that hiredis should retry when the system call was interrupted as EINTR doesn't imply an error state.

Since this has been the functionality for so long we can't really change it. One possible solution would be to create a SIGINT/SIGTERM handler where you could set the redisReader error state, although that's kind of messy.

Another possible solution would be if we added an option that would configure redisContext to treat EINTR as an error.

It's also possible to just override the redisContext->funcs->read with your own function that handles EINTR differently. People do this a fair bit, but right now there's not a nice API to override the functions, and they are defined const.

[RP991]

On Tue, Jan 24, 2023 at 01:29:11PM -0800, Michael Grunder wrote: Thanks for the report. It looks like this change was made very early in the project (see: #99). It appears that the consensus was that hiredis should retry when the system call was interrupted as `EINTR` doesn't imply an error state. Since this has been the functionality for so long we can't really change it. One possible solution would be to create a `SIGINT`/`SIGTERM` handler where you could set the `redisReader` error state, although that's kind of messy. Another possible solution would be if we added an option that would configure `redisContext` to treat `EINTR` as an error. It's also possible to just override the `redisContext->funcs->read` with your own function that handles `EINTR` differently. People do this a fair bit, but right now there's not a nice API to override the functions, and they are defined `const`.

Thanks for the quick response, Michael. I understand the reluctance to alter existing behaviour, and its a trivial thing for me to fix it for our case, but I can't really envisage where the current behaviour would be desirable. I.e. when is an uninterruptable blocking operation a good, or expected, thing? As you say, its been like this for quite some time. I guess I'm surprised to be the first person to complain! Thank you for contributing to such an awesome project. Regards,

…

--Richard