Made some adjustments to have it work for all the "we wait for sth from backend" and added as r1878433 in trunk.
> Am 02.06.2020 um 09:37 schrieb Stefan Eissing <[email protected]>: > > Looks good! > >> Am 29.05.2020 um 21:14 schrieb Ruediger Pluem <[email protected]>: >> >> >> >> On 5/29/20 11:41 AM, Stefan Eissing wrote: >>> >>> >>>> Am 29.05.2020 um 11:23 schrieb Ruediger Pluem <[email protected]>: >>>> >>>> >>>> >>>> On 5/29/20 10:09 AM, Stefan Eissing wrote: >>>>> Looks good. Now I learned about the "ping" parameter... >>>> >>>> Committed as r1878264 with a tweaked comment to make clear what I do. >> >> Thanks for backporting both >> >>>> >>>> Getting me to the next possible enhancement. I already had a patch but >>>> when putting it to the mail I got doubts that it could work >>>> due to the fact, that in most use cases HTTP/2 is encrypted. >>>> In AJP a set ping parameter on the worker will also cause an AJP ping to >>>> be sent as the first thing even on fresh connections and >>>> we only wait for the timeout set in the parameter for a reply. The idea >>>> behind this is that the backend might be able to deal with >>>> a TCP handshake, but not with processing a request, because maybe all >>>> processing threads / processes on the backend application >>>> are busy. This way this can be detected quickly and early and we can sent >>>> our request to a different backend in case of a load >>>> balancing scenario. >>>> With HTTP/2 we will likely have a TLS handshake first which likely already >>>> requires a responding backend application. So it would >>>> not work to wait only for ping timeout time on a ping reply as we will >>>> already wait for the timeout set on the socket to get an >>>> answer to our TLS client Hello. So the idea would be to already lower the >>>> socket timeout to ping timeout before the TLS handshake >>>> starts and reset it back after we received the ping from the backend. >>>> Opinions? >>> >>> HTTP/2 also has an initial SETTINGS frame handshake. We could use the ping >>> timeout on the socket until the first NGHTTP2_SETTINGS frame from the >>> backend arrives on a new connection. >> >> And now I learned about the HTTP/2 handshake :-). I haven't though about the >> SETTINGS frame. So the high level idea would be: >> >> 1. If a fresh session in h2_proxy_session_setup is created and the ping >> parameter is set on the worker change the socket timeout >> to the one in ping, but tuck away the current socket timeout. >> 2. In on_frame_recv in the NGHTTP2_SETTINGS restore the old timeout (if it >> was changed) and continue. >> >> So something like the below? >> >> Index: modules/http2/h2_proxy_session.c >> =================================================================== >> --- modules/http2/h2_proxy_session.c (revision 1878265) >> +++ modules/http2/h2_proxy_session.c (working copy) >> @@ -203,6 +203,21 @@ >> case NGHTTP2_PUSH_PROMISE: >> break; >> case NGHTTP2_SETTINGS: >> + /* >> + * Check if we have saved a socket timeout before sending the >> + * SETTINGS frame in h2_proxy_session_setup to perform a quick >> + * "ping" on the backend. If yes, restore the saved timeout to >> + * the socket. >> + */ >> + if (session->save_timeout != -1) { >> + apr_socket_t *socket = NULL; >> + >> + socket = ap_get_conn_socket(session->c); >> + if (socket) { >> + apr_socket_timeout_set(socket, session->save_timeout); >> + session->save_timeout = -1; >> + } >> + } >> if (frame->settings.niv > 0) { >> n = nghttp2_session_get_remote_settings(ngh2, >> NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS); >> if (n > 0) { >> @@ -634,6 +649,8 @@ >> session->input = apr_brigade_create(session->pool, >> session->c->bucket_alloc); >> session->output = apr_brigade_create(session->pool, >> session->c->bucket_alloc); >> >> + session->save_timeout = -1; >> + >> nghttp2_session_callbacks_new(&cbs); >> nghttp2_session_callbacks_set_on_frame_recv_callback(cbs, >> on_frame_recv); >> nghttp2_session_callbacks_set_on_data_chunk_recv_callback(cbs, >> stream_response_data); >> @@ -654,6 +671,22 @@ >> nghttp2_option_del(option); >> nghttp2_session_callbacks_del(cbs); >> >> + /* >> + * If a ping parameter is set on the worker set the socket timeout >> to >> + * it to "use" the possible TLS handshake and the initial SETTINGS >> + * frame as kind of ping. Tuck away the old timeout to restore it, >> once >> + * the SETTING frame arrived from the backend. >> + */ >> + if (p_conn->worker->s->ping_timeout_set) { >> + apr_socket_t *socket = NULL; >> + >> + socket = ap_get_conn_socket(session->c); >> + if (socket) { >> + apr_socket_timeout_get(socket, &session->save_timeout); >> + apr_socket_timeout_set(socket, >> p_conn->worker->s->ping_timeout); >> + } >> + } >> + >> ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, APLOGNO(03362) >> "setup session for %s", p_conn->hostname); >> } >> Index: modules/http2/h2_proxy_session.h >> =================================================================== >> --- modules/http2/h2_proxy_session.h (revision 1878265) >> +++ modules/http2/h2_proxy_session.h (working copy) >> @@ -94,6 +94,8 @@ >> >> apr_bucket_brigade *input; >> apr_bucket_brigade *output; >> + >> + apr_time_t save_timeout; >> }; >> >> h2_proxy_session *h2_proxy_session_setup(const char *id, proxy_conn_rec >> *p_conn, >> >> >> Regards >> >> Rüdiger >
