Lua's fake requests remain uncleaned in successful cases

[Dslt]

Recently I have had difficulties with such error:
2017/06/07 04:10:42 [alert] 4579#0: *35005 1024 worker_connections are not enough while connecting to upstream

What I found out:
If I'm right, the connections do not close in successful cases in all ngx_http_lua_ssl*_ modules.

Usually connections are created:

fc = ngx_http_lua_create_fake_connection(NULL);

However, the cleaning function is only called in unsuccessful cases:

failed:

    if (r && r->pool) {
        ngx_http_lua_free_fake_request(r);
    }

    if (fc) {
        ngx_http_lua_close_fake_connection(fc);
    }

In successful cases, a handler just return a value:

    if (rc >= NGX_OK || rc == NGX_ERROR) {
        cctx->done = 1;
        ...
        ngx_log_debug2(...);
        ...
        return <something "good">;
    }

Maybe I don't understand something in the architecture, but inserting the cleanup code before returning the "good" value helped me.

[agentzh]

I don't think you are looking at the whole picture. At any rate, it should be freed by the pool cleanup handler.

[agentzh]

Will you create a pull request for it? It would be much easier for us to evaluate. Thanks!

[dndx]

@Dslt Could you please provide a minimum example and steps that shows the above
worker_connections are not enough while connecting to upstream behavior? That will aids the debugging a lot. Thanks.

[Dslt]

Well, I'll do it by the end of the week.

[skyever]

@Dslt @agentzh If in successful case, it means ngx_http_lua_ssl_cert_by_chunk has been called. At the last of this function, the ngx_http_lua_finalize_request will be called. At any rate, the fc will be freed there.
But if some bad thing happens in ngx_http_lua_ssl_cert_by_chunk, such as

`
453 if (ctx == NULL) {
454 ctx = ngx_http_lua_create_ctx(r);
455 if (ctx == NULL) {
456 return NGX_HTTP_INTERNAL_SERVER_ERROR;
457 }
458
459 } else {

`
it returns NGX_HTTP_INTERNAL_SERVER_ERROR, the ngx_http_lua_ssl_cert_handler seems not free the fc.

[agentzh]

@skyever Right, in those code paths, we should replace

return NGX_HTTP_INTERNAL_SERVER_ERROR;

with

rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
ngx_http_lua_finalize_request(r, rc);
return rc;

[agentzh]

@Dslt Will you confirm the code paths your problematic connections actually take inside the lscf->srv.ssl_cert_handler() call?

[skyever]

@agentzh I don't think this is enough. When ngx_http_lua_ssl_cert_by_chunk returns NGX_HTTP_INTERNAL_SERVER_ERROR directly, the ngx_http_lua_ssl_cert_handler will return 1 to openssl, which means successful:

 303     if (rc >= NGX_OK || rc == NGX_ERROR) {
 304         cctx->done = 1;
 305
 306         if (cctx->cleanup) {
 307             *cctx->cleanup = NULL;
 308         }
 309
 310         ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
 311                        "lua_certificate_by_lua: handler return value: %i, "
 312                        "cert cb exit code: %d", rc, cctx->exit_code);
 313
 314         c->log->action = "SSL handshaking";
 315         return cctx->exit_code;
 316     }

The cctx->exit_code is set to 1 by default.

How about to let ngx_http_lua_ssl_cert_by_chunk to return NGX_ERROR when some bad thing happens and let ngx_http_lua_ssl_cert_handler to return to openssl 0 when rc is NGX_ERROR?

[agentzh]

@skyever Yeah, that sounds better. 500 makes no sense in this phase anyway.

[Dslt]

@agentzh I mainly experimented with "sess_fetch_handler".
To my shame, I haven't repeat this, trying to make a pure example from scratch.
Most likely, this problem was a consequence of my own mistakes or, less likely, a bug that I could not catch, similar to the one @skyever described above.
I'm sorry that I wasn't able to become useful.
Thank you for your attention and support.

[agentzh]

@Dslt Thanks for your experiments and feedback!