You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by jo...@apache.org on 2023/12/20 15:56:15 UTC
svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c
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
Added:
httpd/httpd/trunk/changes-entries/flushing-chunks.txt
Modified:
httpd/httpd/trunk/modules/http/chunk_filter.c
Added: httpd/httpd/trunk/changes-entries/flushing-chunks.txt
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/flushing-chunks.txt?rev=1914804&view=auto
==============================================================================
--- httpd/httpd/trunk/changes-entries/flushing-chunks.txt (added)
+++ httpd/httpd/trunk/changes-entries/flushing-chunks.txt Wed Dec 20 15:56:15 2023
@@ -0,0 +1,2 @@
+ *) http/1.1: For a chunked response body, ensure the last-chunk
+ terminator is flushed if necessary. [Joe Orton]
Modified: httpd/httpd/trunk/modules/http/chunk_filter.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/chunk_filter.c?rev=1914804&r1=1914803&r2=1914804&view=diff
==============================================================================
--- 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;
+
+ /* 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))) {
+ 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);
}
Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Dec 20, 2023 at 5:50 PM Joe Orton <jo...@redhat.com> wrote:
>
> On Wed, 20 Dec 2023, 16:30 Yann Ylavic, <yl...@gmail.com> 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
Posted by Joe Orton <jo...@redhat.com>.
On Wed, 20 Dec 2023, 16:30 Yann Ylavic, <yl...@gmail.com> wrote:
> On Wed, Dec 20, 2023 at 5:20 PM Yann Ylavic <yl...@gmail.com> wrote:
> >
> > On Wed, Dec 20, 2023 at 4:56 PM <jo...@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. */
> --
> ?
>
>
Re: svn commit: r1914804 - in /httpd/httpd/trunk: changes-entries/flushing-chunks.txt modules/http/chunk_filter.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Dec 20, 2023 at 5:20 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Wed, Dec 20, 2023 at 4:56 PM <jo...@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:
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
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Dec 20, 2023 at 4:56 PM <jo...@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);
> }
Regards;
Yann.