hiredis
hiredis copied to clipboard
redis
Fix ProtocolError
This PR fixes an issue with incorrect behaviour of redisBufferWrite after merging SSL support.
The following logic was moved from redisBufferWrite to rawWrite in commit
if ((errno == EAGAIN && !(c->flags & REDIS_BLOCK)) || (errno == EINTR)) {
/* Try again later */
} else {
__redisSetError(c, REDIS_ERR_IO, NULL);
return -1;
}
The "Try again later" thing stopped working after that commit. This produces runtime errors in hiredis-rb after upgrading its hiredis to the latest version.
To reproduce the error:
- git clone https://github.com/appcast-inc/hiredis-rb.git (my fork)
- git checkout 66d0807b26b807012193fa5d08c9944d9d982f01 (undo the commit which fixes the error above)
- bundle install && bundle exec rake compile
- and finally, run several times bundle exec rake test
And you will get a runtime error!
Reach me out if you have any questions.
@michael-grunder hello, any change this PR can be merged? or maybe some additional work needed? thank you
Hi @YegorZdanovich
I need to play around with this a bit more to completely think through the right place to catch EAGAIN. The reason it's a bit tricky is that our write function is now abstracted behind a function pointer.
/* This will likely continue to work for any reasonable `write` pointer, but there is no guarantee
`c->funcs->write` will set `errno` as `write` and `send` do. */
ssize_t nwritten = c->funcs->write(c);
if (nwritten < 0) {
if ((errno == EWOULDBLOCK && !(c->flags & REDIS_BLOCK)) || (errno == EINTR)) {
/* Try again later */
} else {
__redisSetError(c, REDIS_ERR_IO, NULL);
return REDIS_ERR;
}
}
We may need to use a special return value from c->funcs->write to specify such a condition generically.