8000 Fix ProtocolError by mtuleika-appcast · Pull Request #961 · redis/hiredis · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix ProtocolError #961

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix ProtocolError #961

wants to merge 1 commit into from

Conversation

mtuleika-appcast
Copy link
Contributor

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:

  1. git clone https://github.com/appcast-inc/hiredis-rb.git (my fork)
  2. git checkout 66d0807b26b807012193fa5d08c9944d9d982f01 (undo the commit which fixes the error above)
  3. bundle install && bundle exec rake compile
  4. and finally, run several times bundle exec rake test

And you will get a runtime error!

Reach me out if you have any questions.

@y-yagi y-yagi mentioned this pull request Nov 7, 2021
Closed
@YegorZdanovich
Copy link

@michael-grunder hello, any change this PR can be merged? or maybe some additional work needed?
thank you

@michael-grunder
Copy link
Collaborator

Apologies, I will test and merge this weekend.

@michael-grunder
Copy link
Collaborator

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.

michael-grunder added a commit to michael-grunder/hiredis that referenced this pull request Sep 4, 2022
@michael-grunder
Attempt to fix protocol error in a generic way
3ab3e87 
This commit attempts to fix hiredis such that a recoverable write error
will be retried rather than throwing a hard error.

Since our read/write functions are now behind function pointers, we
specify semantically that a return value of < 0 is a hard error, 0 a
recoverable error, and > 0 a success.

Our default `redisNetRead` function was already doing something similar
so this also improves code consistency.

See redis#961
@michael-grunder michael-grunder mentioned this pull request Sep 4, 2022
Merged
@michael-grunder
Copy link
Collaborator

Hi, @mtuleika-appcast can you take a look at #1106.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging t 39BF his pull request may close these issues.
3 participants
@mtuleika-appcast @YegorZdanovich @michael-grunder
0