On Wed, 20 Dec 2023, 16:30 Yann Ylavic, <ylavic....@gmail.com> wrote:
> On Wed, Dec 20, 2023 at 5:20 PM Yann Ylavic <ylavic....@gmail.com> wrote: > > > > On Wed, Dec 20, 2023 at 4:56 PM <jor...@apache.org> wrote: > > > > > > Author: jorton > > > Date: Wed Dec 20 15:56:15 2023 > > > New Revision: 1914804 > > > > > > URL: http://svn.apache.org/viewvc?rev=1914804&view=rev > > > Log: > > > * modules/http/chunk_filter.c (ap_http_chunk_filter): For a brigade > > > containing [FLUSH EOS], insert the last-chunk terminator before the > > > FLUSH rather than between the FLUSH and the EOS. > > > > > > Github: closes #400 > > > > It seems that we should set the last-chunk before the FLUSH only if > > the latter precedes an EOS? > > > > So below: > > > > > --- httpd/httpd/trunk/modules/http/chunk_filter.c (original) > > > +++ httpd/httpd/trunk/modules/http/chunk_filter.c Wed Dec 20 15:56:15 > 2023 > > > @@ -64,7 +64,7 @@ apr_status_t ap_http_chunk_filter(ap_fil > > > > > > for (more = tmp = NULL; b; b = more, more = NULL) { > > > apr_off_t bytes = 0; > > > - apr_bucket *eos = NULL; > > > + apr_bucket *eos = NULL; /* EOS bucket, or FLUSH preceding EOS > */ > > > apr_bucket *flush = NULL; > > > /* XXX: chunk_hdr must remain at this scope since it is used > in a > > > * transient bucket. > > > @@ -99,8 +99,20 @@ apr_status_t ap_http_chunk_filter(ap_fil > > > } > > > if (APR_BUCKET_IS_FLUSH(e)) { > > > flush = e; > > > > Don't set "flush" above. > > > > > + > > > + /* Special case to catch common brigade ending of > > > + * [FLUSH] [EOS] - insert the last_chunk before > > > + * the FLUSH rather than between the FLUSH and the > > > + * EOS. */ > > > if (e != APR_BRIGADE_LAST(b)) { > > > - more = apr_brigade_split_ex(b, > APR_BUCKET_NEXT(e), tmp); > > > + if (APR_BUCKET_IS_EOS(APR_BUCKET_NEXT(e))) { > > > > But here only? > > > > > + eos = e; > > > + /* anything after EOS is dropped, no need > > > + * to split. */ > > > + } > > > + else { > > > + more = apr_brigade_split_ex(b, > APR_BUCKET_NEXT(e), tmp); > > > + } > > > } > > > break; > > > } > > > @@ -173,12 +185,12 @@ apr_status_t ap_http_chunk_filter(ap_fil > > > * FLUSH bucket, or appended to the brigade > > > */ > > > e = apr_bucket_immortal_create(CRLF_ASCII, 2, > c->bucket_alloc); > > > - if (eos != NULL) { > > > - APR_BUCKET_INSERT_BEFORE(eos, e); > > > - } > > > - else if (flush != NULL) { > > > + if (flush != NULL) { > > > APR_BUCKET_INSERT_BEFORE(flush, e); > > > } > > > + else if (eos != NULL) { > > > + APR_BUCKET_INSERT_BEFORE(eos, e); > > > + } > > > else { > > > APR_BRIGADE_INSERT_TAIL(b, e); > > > } > > Ah no, this is for every chunk so we are good here. > For the last-chunk, I think we need > I think that's not necessary because in the special case eos=flush, in the normal case flush is NULL. Actually the reordering of the tests above doesn't make any difference either, I could have skipped that. In the first iteration of the patch I renamed eos to "terminator" to make it more obvious. Index: modules/http/chunk_filter.c > =================================================================== > --- modules/http/chunk_filter.c (revision 1914804) > +++ modules/http/chunk_filter.c (working copy) > @@ -217,7 +217,7 @@ apr_status_t ap_http_chunk_filter(ap_filter_t *f, > * now. > */ > if (eos && !ctx->bad_gateway_seen) { > - ap_h1_add_end_chunk(b, eos, f->r, ctx->trailers); > + ap_h1_add_end_chunk(b, flush ? flush : eos, f->r, > ctx->trailers); > } > > /* pass the brigade to the next filter. */ > -- > ? > >
