> Am 27.05.2020 um 17:58 schrieb Ruediger Pluem <[email protected]>: > > > > On 5/27/20 5:25 PM, Stefan Eissing wrote: >> Maybe this can work? It goes into blocking read with default timeout when >> there is definitely nothing to send from our end. >> >> >> h2-proxy-timeout.patch >> >> Index: modules/http2/h2_proxy_session.c >> =================================================================== >> --- modules/http2/h2_proxy_session.c (Revision 1878161) >> +++ modules/http2/h2_proxy_session.c (Arbeitskopie) >> @@ -556,11 +556,14 @@ >> "h2_proxy_stream(%d): request DATA %ld, %ld" >> " total, flags=%d", stream->id, (long)readlen, >> (long)stream->data_sent, >> (int)*data_flags); >> - if ((*data_flags & NGHTTP2_DATA_FLAG_EOF) && >> !apr_is_empty_table(stream->r->trailers_in)) { >> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, stream->r, >> APLOGNO(10179) >> - "h2_proxy_stream(%d): submit trailers", >> stream->id); >> - *data_flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; >> - submit_trailers(stream); >> + if (*data_flags & NGHTTP2_DATA_FLAG_EOF) { >> + if (!apr_is_empty_table(stream->r->trailers_in)) { >> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, stream->r, >> APLOGNO(10179) >> + "h2_proxy_stream(%d): submit trailers", >> stream->id); >> + *data_flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; >> + submit_trailers(stream); >> + } >> + stream->state = H2_STREAM_ST_CLOSED_INPUT; > > Where is this state processed? Looks to me as if this state would fall into > the default case of the switch in > h2_proxy_session_process and thus cause an error messages. But as far as I > understand this only indicates that the front does not > want to sent more data which is no error.
I removed this again. mod_http2 tracks stream states more closely that
proxy_http2 and I thought it would help. But we can do without, I think.
>> }
>> return readlen;
>> }
>> @@ -902,7 +905,7 @@
>> apr_socket_t *socket = NULL;
>> apr_time_t save_timeout = -1;
>>
>> - if (block) {
>> + if (block && timeout > 0) {
>> socket = ap_get_conn_socket(session->c);
>> if (socket) {
>> apr_socket_timeout_get(socket, &save_timeout);
>> @@ -974,6 +977,11 @@
>> dispatch_event(session, H2_PROXYS_EV_STREAM_RESUMED, 0, NULL);
>> }
>>
>> +static int has_suspended_streams(h2_proxy_session *session)
>> +{
>> + return (session->suspended->nelts > 0);
>> +}
>> +
>> static apr_status_t check_suspended(h2_proxy_session *session)
>> {
>> h2_proxy_stream *stream;
>> @@ -1428,7 +1436,20 @@
>> break;
>>
>> case H2_PROXYS_ST_WAIT:
>> - if (check_suspended(session) == APR_EAGAIN) {
>> + if (!has_suspended_streams(session)
>> + && !nghttp2_session_want_write(session->ngh2)
>> + && nghttp2_session_want_read(session->ngh2)) {
>> + status = h2_proxy_session_read(session, 1, 0);
>> + if (status == APR_SUCCESS) {
>> + have_read = 1;
>> + dispatch_event(session, H2_PROXYS_EV_DATA_READ, 0,
>> NULL);
>> + }
>> + else {
>> + dispatch_event(session, H2_PROXYS_EV_CONN_ERROR,
>> status, NULL);
>> + return status;
>> + }
>> + }
>
> Hm. I think this does not catch the case where we sent a PING to the backend
> and are waiting for the reply because we might have
> suspended streams in this case?
> So maybe instead of
>
> !has_suspended_streams(session)
>
> we need to check for
>
> (!has_suspended_streams(session) || session->check_ping)
>
> ?
You are correct. I made a v2 of the patch:
h2-proxy-timeout-v2.patch
Description: Binary data
Cheers, Stefan
