On 7/2/22 11:39 AM, ic...@apache.org wrote: > Author: icing > Date: Sat Jul 2 09:39:22 2022 > New Revision: 1902409 > > URL: http://svn.apache.org/viewvc?rev=1902409&view=rev > Log: > *) mod_http2: fixed trailer handling. Empty response bodies > prevented trailers from being sent to a client. See > <https://github.com/icing/mod_h2/issues/233> for how > this affected gRPC use. > > > Added: > httpd/httpd/trunk/changes-entries/h2_trailers.txt > Modified: > httpd/httpd/trunk/modules/http2/h2_stream.c > httpd/httpd/trunk/modules/http2/h2_stream.h > > Modified: httpd/httpd/trunk/modules/http2/h2_stream.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_stream.c?rev=1902409&r1=1902408&r2=1902409&view=diff > ============================================================================== > --- httpd/httpd/trunk/modules/http2/h2_stream.c (original) > +++ httpd/httpd/trunk/modules/http2/h2_stream.c Sat Jul 2 09:39:22 2022 > @@ -1406,60 +1412,81 @@ static ssize_t stream_data_cb(nghttp2_se > } > > /* How much data do we have in our buffers that we can write? */ > - buf_len = output_data_buffered(stream, &eos); > - if (buf_len < length && !eos) { > +check_and_receive: > + buf_len = output_data_buffered(stream, &eos, &header_blocked); > + while (buf_len < length && !eos && !header_blocked) { > /* read more? */ > ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c1, > H2_SSSN_STRM_MSG(session, stream_id, > "need more (read len=%ld, %ld in buffer)"), > (long)length, (long)buf_len); > rv = buffer_output_receive(stream); > - /* process all headers sitting at the buffer head. */ > - while (APR_SUCCESS == rv && !eos && !stream->sent_trailers) { > - rv = buffer_output_process_headers(stream); > - if (APR_SUCCESS != rv && APR_EAGAIN != rv) { > - ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c1, > - H2_STRM_LOG(APLOGNO(10300), stream, > - "data_cb, error processing headers")); > - return NGHTTP2_ERR_CALLBACK_FAILURE; > - } > - buf_len = output_data_buffered(stream, &eos); > + if (APR_EOF == rv) { > + eos = 1; > + rv = APR_SUCCESS; > } > > - if (APR_SUCCESS != rv && !APR_STATUS_IS_EAGAIN(rv)) { > + if (APR_SUCCESS == rv) { > + /* re-assess */ > + buf_len = output_data_buffered(stream, &eos, &header_blocked); > + } > + else if (APR_STATUS_IS_EAGAIN(rv)) { > + /* currently, no more is available */ > + break; > + } > + else if (APR_SUCCESS != rv) { The if is always true as if APR_SUCCESS == rv we hit the first block. > ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c1, > H2_STRM_LOG(APLOGNO(02938), stream, "data_cb, > reading data")); > return NGHTTP2_ERR_CALLBACK_FAILURE; > } > + } > > - if (stream->sent_trailers) { > - AP_DEBUG_ASSERT(eos); > - AP_DEBUG_ASSERT(buf_len == 0); > - return NGHTTP2_ERR_DEFERRED; > + if (buf_len == 0 && header_blocked) { > + /* we are blocked from having data to send by a HEADER bucket sitting > + * at buffer start. Send it and check again what DATA we can send. */ > + rv = buffer_output_process_headers(stream); > + if (APR_SUCCESS == rv) { > + goto check_and_receive; > + } > + else if (APR_STATUS_IS_EAGAIN(rv)) { > + /* unable to send the HEADER at this time. */ > + eos = 0; > + goto cleanup; > + } > + else { > + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c1, > + H2_STRM_LOG(APLOGNO(10300), stream, > + "data_cb, error processing headers")); > + return NGHTTP2_ERR_CALLBACK_FAILURE; > } > } > > if (buf_len > (apr_off_t)length) { > - eos = 0; > + eos = 0; /* Any EOS we have in the buffer does not apply yet */ > } > else { > length = (size_t)buf_len; > } > + > + if (stream->sent_trailers) { > + /* We already sent trailers and will/can not send more DATA. */ > + eos = 0; > + } > + > if (length) { > ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c1, > H2_STRM_MSG(stream, "data_cb, sending len=%ld, > eos=%d"), > (long)length, eos); > *data_flags |= NGHTTP2_DATA_FLAG_NO_COPY; > } > - else if (!eos) { > - /* no data available and output is not closed, need to suspend */ > + else if (!eos && !stream->sent_trailers) { > + /* We have not reached the end of DATA yet, DEFER sending */ > ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c1, > H2_STRM_LOG(APLOGNO(03071), stream, "data_cb, > suspending")); > - ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, c1, > - H2_SSSN_STRM_MSG(session, stream_id, "suspending")); > return NGHTTP2_ERR_DEFERRED; > } > > +cleanup: > if (eos) { > *data_flags |= NGHTTP2_DATA_FLAG_EOF; > } > @@ -1502,7 +1529,7 @@ apr_status_t h2_stream_read_output(h2_st > } > > rv = buffer_output_receive(stream); > - if (APR_SUCCESS != rv) goto cleanup; > + if (APR_SUCCESS != rv && APR_EAGAIN != rv) goto cleanup; Why not using !APR_STATUS_IS_EAGAIN(rv) ? > > /* process all headers sitting at the buffer head. */ > while (1) { > Regards Rüdiger
