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
