Skip to content
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.

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

issue with long replies #9

Open
vlabel opened this issue Oct 14, 2020 · 3 comments
Open

issue with long replies #9

vlabel opened this issue Oct 14, 2020 · 3 comments

Comments

@vlabel
Copy link

vlabel commented Oct 14, 2020

looks like there is a problem with big responses - more than char uf[1024*16];
https://github.com/redis/hiredis/blob/acc917548dcfd91e5d56387ed58b5f408cdb810a/hiredis.c#L933

Imagine we get response from SCAN or GET which is longer than 1024*16
The problem is that redis_nginx_read_event will be called only once - and hiredis redisBufferRead will read only part of data
a new epoll event will not be registered -> because connection->read->active == 1

Looks like if connection->read->active is true - we need to call ngx_del_event before adding it in redis_nginx_add_read

Got this problem with unix socket used

@vlabel vlabel changed the title issue on big reply on SCAN issue with long replies Oct 14, 2020
@wandenberg
Copy link
Owner

Hi @vlabel. Can you help me to reproduce the issue with some docker/vagrant or curl commands?
It was not clear to me if the mentioned problem is on hiredis or on redis_nginx_adapter.

@vlabel
Copy link
Author

vlabel commented Oct 18, 2020

Hi, @wandenberg Thanks for answering this question.

Here is a reproduce.

  1. Change 1 line in the existing example module -> swap GET with SCAN 0 COUNT 100
    (We want to get response longer than 16k)
diff --git a/example/module/ngx_redis_adapter_example_module.c b/example/module/ngx_redis_adapter_example_module.c
index 6fc1ad8..30de0f6 100755
--- a/example/module/ngx_redis_adapter_example_module.c
+++ b/example/module/ngx_redis_adapter_example_module.c
@@ -157,7 +157,7 @@ ngx_redis_adapter_example_request_handler(ngx_http_request_t *r)
     ngx_str_t     vv_value = ngx_null_string;
 
     if (r->method & NGX_HTTP_GET) {
-        redisAsyncCommand(redis_server, get_callback, r, "GET %b", r->uri.data, r->uri.len);
+        redisAsyncCommand(redis_server, get_callback, r, "SCAN 0 COUNT 100");
     } else if (r->method & NGX_HTTP_POST) {
         if ((ngx_http_arg(r, (u_char *) "v", 1, &vv_value) == NGX_OK) && (vv_value.len > 0)) {
             redisAsyncCommand(redis_server, set_callback, r, "SET %b %b", r->uri.data, r->uri.len, vv_value.data, vv_value.len);
  1. build and run example container

  2. Fill redis with test data - total amount longer 16k
    for i in {1..100}; do curl -v -X POST curl localhost:8001/"dDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDdddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd$i?v=111"; done

  3. initiate SCAN
    curl localhost:8002/123
    unfortunately this reproduce is not 100% stable - probably because the response is not always generate exactly one epoll event.
    But if you try to run
    curl localhost:8002/123 several times (for me 1 of 4) - it will stuck -> without response. (if you get 204 -> it works; if your request just stuck -> that mean we got the issue)

@vlabel
Copy link
Author

vlabel commented Oct 18, 2020

honestly I don't understand why hiredis just reads a buffer once and don't check if there is more data.
probably they just rely on a fact that the read event will be called again. That is why they reschedule read here. https://github.com/redis/hiredis/blob/master/async.c#L639

If so -> why several existing adapters https://github.com/redis/hiredis/tree/master/adapters (and your) just skips add_read if event is active ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants