Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c
On Wed, Dec 20, 2023 at 5:50 PM Joe Orton wrote: > > On Wed, 20 Dec 2023, 16:30 Yann Ylavic, wrote: >> >> 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. You are right, I missed that "eos = e" and not "eos = APR_BUCKET_NEXT(e)" in this case. My noise, all good! Regards; Yann.
Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c
On Wed, 20 Dec 2023, 16:30 Yann Ylavic, wrote: > On Wed, Dec 20, 2023 at 5:20 PM Yann Ylavic wrote: > > > > On Wed, Dec 20, 2023 at 4:56 PM wrote: > > > > > > Author: jorton > > > Date: Wed Dec 20 15:56:15 2023 > > > New Revision: 1914804 > > > > > > URL: http://svn.apache.org/viewvc?rev=1914804=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. */ > -- > ? > >
Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c
On Wed, Dec 20, 2023 at 5:20 PM Yann Ylavic wrote: > > On Wed, Dec 20, 2023 at 4:56 PM wrote: > > > > Author: jorton > > Date: Wed Dec 20 15:56:15 2023 > > New Revision: 1914804 > > > > URL: http://svn.apache.org/viewvc?rev=1914804=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: 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. */ -- ?
Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c
On Wed, Dec 20, 2023 at 4:56 PM wrote: > > Author: jorton > Date: Wed Dec 20 15:56:15 2023 > New Revision: 1914804 > > URL: http://svn.apache.org/viewvc?rev=1914804=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); > } Regards; Yann.