Permalink
Browse files

bugfix: ngx.thread.wait() might hang infinitely when more than 4 user…

… "light threads" are created in the same request handler due to the incorrect use of ngx_array_t for ngx_list_t. thanks Junwei Shi for reporting this issue.
  • Loading branch information...
agentzh committed Nov 29, 2012
1 parent 5abd74c commit 2ecf5afa8336729a84a3ec72cc6c044c2db886b5
Showing with 244 additions and 30 deletions.
  1. +1 −1 src/ngx_http_lua_common.h
  2. +72 −28 src/ngx_http_lua_util.c
  3. +171 −1 t/093-uthread-spawn.t
@@ -274,7 +274,7 @@ typedef struct ngx_http_lua_ctx_s {
ngx_http_lua_co_ctx_t *cur_co_ctx; /* co ctx for the current coroutine */
/* FIXME: we should use rbtree here to prevent O(n) lookup overhead */
- ngx_array_t *user_co_ctx; /* coroutine contexts for user
+ ngx_list_t *user_co_ctx; /* coroutine contexts for user
coroutines */
ngx_http_lua_co_ctx_t entry_co_ctx; /* coroutine context for the
View
@@ -324,6 +324,7 @@ ngx_http_lua_del_all_threads(ngx_http_request_t *r, lua_State *L,
int inited = 0;
int ref;
ngx_uint_t i;
+ ngx_list_part_t *part;
ngx_http_lua_co_ctx_t *entry_coctx;
ngx_http_lua_co_ctx_t *cc;
@@ -358,9 +359,20 @@ ngx_http_lua_del_all_threads(ngx_http_request_t *r, lua_State *L,
inited = 1;
}
- cc = ctx->user_co_ctx->elts;
+ part = &ctx->user_co_ctx->part;
+ cc = part->elts;
- for (i = 0; i < ctx->user_co_ctx->nelts; i++) {
+ for (i = 0; /* void */; i++) {
+
+ if (i >= part->nelts) {
+ if (part->next == NULL) {
+ break;
+ }
+
+ part = part->next;
+ cc = part->elts;
+ i = 0;
+ }
ref = cc[i].co_ref;
@@ -845,7 +857,7 @@ ngx_http_lua_reset_ctx(ngx_http_request_t *r, lua_State *L,
ngx_http_lua_del_all_threads(r, L, ctx);
if (ctx->user_co_ctx) {
- ngx_array_destroy(ctx->user_co_ctx);
+ /* no way to destroy a list but clean up the whole pool */
ctx->user_co_ctx = NULL;
}
@@ -1448,6 +1460,7 @@ ngx_int_t
ngx_http_lua_wev_handler(ngx_http_request_t *r)
{
ngx_int_t rc;
+ ngx_list_part_t *part;
ngx_http_lua_ctx_t *ctx;
ngx_connection_t *c;
ngx_event_t *wev;
@@ -1589,32 +1602,37 @@ ngx_http_lua_wev_handler(ngx_http_request_t *r)
return NGX_ERROR;
}
- coctx = ctx->user_co_ctx->elts;
+ part = &ctx->user_co_ctx->part;
+ coctx = part->elts;
- i = 0;
- do {
- for ( ;; ) {
- if (i >= ctx->user_co_ctx->nelts) {
- return NGX_ERROR;
- }
+ for (i = 0; /* void */; i++) {
- if (coctx[i].flushing) {
- coctx[i].flushing = 0;
- ctx->cur_co_ctx = &coctx[i];
+ if (i >= part->nelts) {
+ if (part->next == NULL) {
break;
}
- i++;
+ part = part->next;
+ coctx = part->elts;
+ i = 0;
}
- rc = ngx_http_lua_flush_resume_helper(r, ctx);
- if (rc == NGX_ERROR || rc >= NGX_OK) {
- return rc;
- }
+ if (coctx[i].flushing) {
+ coctx[i].flushing = 0;
+ ctx->cur_co_ctx = &coctx[i];
+
+ rc = ngx_http_lua_flush_resume_helper(r, ctx);
+ if (rc == NGX_ERROR || rc >= NGX_OK) {
+ return rc;
+ }
- /* rc == NGX_DONE */
+ /* rc == NGX_DONE */
- } while (--ctx->flushing_coros);
+ if (--ctx->flushing_coros == 0) {
+ break;
+ }
+ }
+ }
}
if (ctx->flushing_coros) {
@@ -2770,6 +2788,7 @@ ngx_http_lua_co_ctx_t *
ngx_http_lua_get_co_ctx(lua_State *L, ngx_http_lua_ctx_t *ctx)
{
ngx_uint_t i;
+ ngx_list_part_t *part;
ngx_http_lua_co_ctx_t *coctx;
if (L == ctx->entry_co_ctx.co) {
@@ -2780,12 +2799,24 @@ ngx_http_lua_get_co_ctx(lua_State *L, ngx_http_lua_ctx_t *ctx)
return NULL;
}
- coctx = ctx->user_co_ctx->elts;
+ part = &ctx->user_co_ctx->part;
+ coctx = part->elts;
/* FIXME: we should use rbtree here to prevent O(n) lookup overhead */
- for (i = 0; i < ctx->user_co_ctx->nelts; i++) {
- if (L == coctx[i].co) {
+ for (i = 0; /* void */; i++) {
+
+ if (i >= part->nelts) {
+ if (part->next == NULL) {
+ break;
+ }
+
+ part = part->next;
+ coctx = part->elts;
+ i = 0;
+ }
+
+ if (coctx[i].co == L) {
return &coctx[i];
}
}
@@ -2800,14 +2831,14 @@ ngx_http_lua_create_co_ctx(ngx_http_request_t *r, ngx_http_lua_ctx_t *ctx)
ngx_http_lua_co_ctx_t *coctx;
if (ctx->user_co_ctx == NULL) {
- ctx->user_co_ctx = ngx_array_create(r->pool, 4,
- sizeof(ngx_http_lua_co_ctx_t));
+ ctx->user_co_ctx = ngx_list_create(r->pool, 4,
+ sizeof(ngx_http_lua_co_ctx_t));
if (ctx->user_co_ctx == NULL) {
return NULL;
}
}
- coctx = ngx_array_push(ctx->user_co_ctx);
+ coctx = ngx_list_push(ctx->user_co_ctx);
if (coctx == NULL) {
return NULL;
}
@@ -2900,6 +2931,7 @@ static void
ngx_http_lua_finalize_coroutines(ngx_http_request_t *r, ngx_http_lua_ctx_t *ctx)
{
ngx_http_lua_co_ctx_t *cc, *coctx;
+ ngx_list_part_t *part;
ngx_uint_t i;
if (ctx->uthreads == 0) {
@@ -2915,9 +2947,21 @@ ngx_http_lua_finalize_coroutines(ngx_http_request_t *r, ngx_http_lua_ctx_t *ctx)
}
if (ctx->user_co_ctx) {
- cc = ctx->user_co_ctx->elts;
+ part = &ctx->user_co_ctx->part;
+ cc = part->elts;
+
+ for (i = 0; /* void */; i++) {
+
+ if (i >= part->nelts) {
+ if (part->next == NULL) {
+ break;
+ }
+
+ part = part->next;
+ cc = part->elts;
+ i = 0;
+ }
- for (i = 0; i < ctx->user_co_ctx->nelts; i++) {
coctx = &cc[i];
if (coctx->cleanup) {
coctx->cleanup(coctx);
View
@@ -9,7 +9,7 @@ our $StapScript = $t::StapThread::StapScript;
repeat_each(2);
-plan tests => repeat_each() * (blocks() * 4 + 1);
+plan tests => repeat_each() * (blocks() * 4);
$ENV{TEST_NGINX_RESOLVER} ||= '8.8.8.8';
$ENV{TEST_NGINX_MEMCACHED_PORT} ||= '11211';
@@ -1470,3 +1470,173 @@ status: 204
[error]
--- timeout: 3
+
+
+=== TEST 29: multiple user threads + subrequests returning 404 remotely (wait)
+--- config
+ location /t {
+ content_by_lua '
+ local n = 5
+ local capture = ngx.location.capture
+ local insert = table.insert
+
+ local function f(i)
+ local res = capture("/proxy/" .. i)
+ return res.status
+ end
+
+ local threads = {}
+ for i = 1, n do
+ local co = ngx.thread.spawn(f, i)
+ insert(threads, co)
+ end
+
+ for i = 1, n do
+ local ok, res = ngx.thread.wait(threads[i])
+ ngx.say(i, ": ", res)
+ end
+
+ ngx.say("ok")
+ ';
+ }
+
+ location ~ ^/proxy/(\d+) {
+ proxy_pass http://127.0.0.1:$server_port/d/$1;
+ }
+
+ location /d {
+ return 404;
+ #echo $uri;
+ }
+--- request
+ GET /t
+--- stap2 eval: $::StapScript
+--- stap3 eval: $::GCScript
+--- stap_out3
+create 2 in 1
+spawn user thread 2 in 1
+create 3 in 1
+spawn user thread 3 in 1
+create 4 in 1
+spawn user thread 4 in 1
+create 5 in 1
+spawn user thread 5 in 1
+create 6 in 1
+spawn user thread 6 in 1
+terminate 2: ok
+delete thread 2
+terminate 3: ok
+delete thread 3
+terminate 4: ok
+delete thread 4
+terminate 5: ok
+delete thread 5
+terminate 6: ok
+delete thread 6
+terminate 1: ok
+delete thread 1
+
+--- response_body
+1: 404
+2: 404
+3: 404
+4: 404
+5: 404
+ok
+--- no_error_log
+[error]
+--- timeout: 3
+
+
+
+=== TEST 30: multiple user threads + subrequests returning 404 remotely (wait)
+--- config
+ location /t {
+ content_by_lua '
+ local n = 20
+ local capture = ngx.location.capture
+ local insert = table.insert
+
+ local function f(i)
+ local res = capture("/proxy/" .. i)
+ return res.status
+ end
+
+ local threads = {}
+ for i = 1, n do
+ local co = ngx.thread.spawn(f, i)
+ insert(threads, co)
+ end
+
+ for i = 1, n do
+ local ok, res = ngx.thread.wait(threads[i])
+ ngx.say(i, ": ", res)
+ end
+
+ ngx.say("ok")
+ ';
+ }
+
+ location ~ ^/proxy/(\d+) {
+ proxy_pass http://127.0.0.1:$server_port/d/$1;
+ }
+
+ location /d {
+ echo_sleep 0.001;
+ echo $uri;
+ }
+--- request
+ GET /t
+--- stap2 eval: $::StapScript
+--- stap3 eval: $::GCScript
+--- stap_out3
+create 2 in 1
+spawn user thread 2 in 1
+create 3 in 1
+spawn user thread 3 in 1
+create 4 in 1
+spawn user thread 4 in 1
+create 5 in 1
+spawn user thread 5 in 1
+create 6 in 1
+spawn user thread 6 in 1
+terminate 2: ok
+delete thread 2
+terminate 3: ok
+delete thread 3
+terminate 4: ok
+delete thread 4
+terminate 5: ok
+delete thread 5
+terminate 6: ok
+delete thread 6
+terminate 1: ok
+delete thread 1
+
+--- response_body
+1: 200
+2: 200
+3: 200
+4: 200
+5: 200
+6: 200
+7: 200
+8: 200
+9: 200
+10: 200
+11: 200
+12: 200
+13: 200
+14: 200
+15: 200
+16: 200
+17: 200
+18: 200
+19: 200
+20: 200
+ok
+--- no_error_log
+[error]
+[alert]
+--- timeout: 3
+

0 comments on commit 2ecf5af

Please sign in to comment.