bugfix: the connection might be closed when some handlers using fake … #1162
Conversation
…connection are involved (like ssl_certficate_by_lua*, ssl_session_fetch_by_lua*, ssl_session_store_by_lua*).
|
@guanglinlv How about the following patch instead? diff --git a/src/ngx_http_lua_ssl_certby.c b/src/ngx_http_lua_ssl_certby.c
index c3591d1e..242724b8 100644
--- a/src/ngx_http_lua_ssl_certby.c
+++ b/src/ngx_http_lua_ssl_certby.c
@@ -197,7 +197,7 @@ ngx_http_lua_ssl_cert_handler(ngx_ssl_conn_t *ssl_conn, void *data)
c = ngx_ssl_get_connection(ssl_conn);
- dd("c = %p", c);
+ dd("c = %p, reusable = %d", c, (int) c->reusable);
cctx = ngx_http_lua_ssl_get_ctx(c->ssl->connection);
@@ -220,6 +220,8 @@ ngx_http_lua_ssl_cert_handler(ngx_ssl_conn_t *ssl_conn, void *data)
dd("first time");
+ ngx_reusable_connection(c, 0);
+
hc = c->data;
fc = ngx_http_lua_create_fake_connection(NULL);
diff --git a/src/ngx_http_lua_ssl_session_fetchby.c b/src/ngx_http_lua_ssl_session_fetchby.c
index 556b7320..a5445626 100644
--- a/src/ngx_http_lua_ssl_session_fetchby.c
+++ b/src/ngx_http_lua_ssl_session_fetchby.c
@@ -191,7 +191,7 @@ ngx_http_lua_ssl_sess_fetch_handler(ngx_ssl_conn_t *ssl_conn, u_char *id,
c = ngx_ssl_get_connection(ssl_conn);
- dd("c = %p", c);
+ dd("c = %p, reusable = %d", c, (int) c->reusable);
cctx = ngx_http_lua_ssl_get_ctx(c->ssl->connection);
@@ -224,6 +224,8 @@ ngx_http_lua_ssl_sess_fetch_handler(ngx_ssl_conn_t *ssl_conn, u_char *id,
dd("first time");
+ ngx_reusable_connection(c, 0);
+
hc = c->data;
fc = ngx_http_lua_create_fake_connection(NULL);
diff --git a/src/ngx_http_lua_ssl_session_storeby.c b/src/ngx_http_lua_ssl_session_storeby.c
index bae8273d..60ef06b7 100644
--- a/src/ngx_http_lua_ssl_session_storeby.c
+++ b/src/ngx_http_lua_ssl_session_storeby.c
@@ -183,7 +183,7 @@ ngx_http_lua_ssl_sess_store_handler(ngx_ssl_conn_t *ssl_conn,
c = ngx_ssl_get_connection(ssl_conn);
- dd("c = %p", c);
+ dd("c = %p, reusable = %d", c, (int) c->reusable);
cctx = ngx_http_lua_ssl_get_ctx(c->ssl->connection); |
|
@guanglinlv I don't think we need to do this in the |
|
@guanglinlv Thank you very much for the report and patch! |
|
@agentzh I think it should be better to let the connection reusable after creating fake connection. I'm not sure about |
|
@guanglinlv Marking it as non-reusable after creating the fake connection does not look quite right to me since the fake connection may drain and hijack the real connection for the same context, which will be very bad. |
|
@agentzh You are right, we just need marking the connection non-reusable before creating the fake connection. I will commit it again following your suggestion. |
…connection are involved (like ssl_certficate_by_lua*,ssl_session_fetch_by_lua*).
|
@guanglinlv You tried my patch above and it works for you, right? |
|
@guanglinlv It's indeed tricky to write test cases for this. |
|
@agentzh I had test it about 3 hours with the maximum concurrency connections that the nginx can handled. the problem is not repeat. I will keep a long time pressure and check it again. BTW, any idea about test case? |
…hen ssl_certificate_by_lua* or ssl_session_fetch_by_lua* were used. this might lead to segmentation faults under load. thanks guanglinlv for the report and the original patch in #1162.
|
@guanglinlv Thanks for the test feedback! Regarding to the regression tests, I ended up inspecting the nginx debug logs in the corresponding test cases. See commit 97fbeb0 for details. |
|
@agentzh Nginx with this patch is running more than ten hours, the problem never occurs once again. |
|
@guanglinlv Cool! |
2 participants
…connection are involved (like ssl_certficate_by_lua*, ssl_session_fetch_by_lua*, ssl_session_store_by_lua*).
fix the issue reported at ngx_http_lua_ssl_cert_handler crash.
it's a little difficult to add a test case to cover this. any idea?
@agentzh
I hereby granted the copyright of the changes in this pull request
to the authors of this lua-nginx-module project.