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

h2 slave connection reuse does not work reliably #128

Closed
arminabf opened this Issue Feb 9, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@arminabf
Copy link

arminabf commented Feb 9, 2017

When doing some tests with the mod_http2 module I'd realized that slave connections (the "cloned" connections from the master connection, used by h2_task's) are mostly (not always) closed after a request - although connection re-use (keep-alive) should be called into action.

So far I observe following behaviour:

When the corresponding handler finishes, ap_check_pipline() is called by ap_process_request_after_handler(). ap_check_pipline() tries to read "blank lines" in AP_MODE_SPECULATIVE mode. This triggers the h2_filter_slave_in() filter that in the majority of cases returns with APR_EOF.

Based on the return code ap_check_pipeline() assumes that the pipe is dead and closes the connection.

else if (rv != APR_SUCCESS || APR_BRIGADE_EMPTY(bb)) {
                if (rv != APR_SUCCESS && !APR_STATUS_IS_EAGAIN(rv)) {
                    /* Pipe is dead */
                    c->keepalive = AP_CONN_CLOSE;
                }   

In the end, when the h2_task is destroyed (in task_destroy()) the slave connection is not set aside for connection reuse but is destroyed and the underlying socket gets closed.

    if (slave) {
      if (reuse_slave && slave->keepalive == AP_CONN_KEEPALIVE) {
        APR_ARRAY_PUSH(m->spare_slaves, conn_rec *) = slave;
      } else {
        slave->sbh = NULL;
        h2_slave_destroy(slave);
      }    
    }

As mentioned, sometimes a slave connection is reused...but very infrequently.

The tests have been made with "mod_http2 (v1.8.3, feats=CHPRIO+SHA256+INVHD, nghttp2 1.18.1)" based on Apache httpd v2.4.25.

@icing

This comment has been minimized.

Copy link
Owner

icing commented Feb 9, 2017

Thanks! Very interesting observation and certainly something that needs fixing.

@icing icing added the enhancement label Feb 14, 2017

@icing icing self-assigned this Feb 14, 2017

@icing

This comment has been minimized.

Copy link
Owner

icing commented Feb 14, 2017

Will be fixed in the upcoming v1.9.0.

@icing

This comment has been minimized.

Copy link
Owner

icing commented Feb 16, 2017

@arminabf can you verify that v1.9.0 works on your setup too? Thanks!

@arminabf

This comment has been minimized.

Copy link
Author

arminabf commented Feb 16, 2017

yes works, thnx!!!

@icing icing closed this Feb 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment