Error bucket is a special kind of meta bucket that is not part of apr but added in httpd and can be created via the ap_bucket_error_create() call. This type of bucket is simply destroyed by mod_xml2enc. I have created a pull request with a fix that resolves this issue: https://github.com/apache/httpd/pull/48
Created attachment 35769 [details] mod_xml2enc.c (xml2enc_ffunc): do not remove error buckets I worked with Mihaly on this issue, and he found the fix. The impact of the error bucket getting dropped is that any failure raised by another filter earlier in the output filter chain will get dropped. This means an entirely broken page is delivered instead of blocking the request with a proper error message.
Created attachment 35770 [details] mod_xml2enc.c: pass all meta buckets down the chain How about passing *all* metadata buckets down the chain (and, btw, bail out on EOS)?
I've just tested your patch, and it fixes our issue with the dropped error bucket too. We didn't dare to do a bigger clean-up like you did, because we were not sure what else could break by doing so. For that reason I've just run our internal charset conversion and HTML rewriting regression test suite against a httpd with your patch applied which flagged no issues. From our perspective your patch is good to go.
Hi Yann, if you commit this please add "this closes #48" to the commit to close out the github PR.
Yann, is there any additional information needed from our side?
I'd like the patch to aggregate meta buckets (besides FLUSH which can't be retained), and then send them together either before further real data or before leaving. I didn't start working on this though, will try soon if you don't beat me at it ;)
Created attachment 35869 [details] mod_xml2enc: pass all meta buckets down the chain (v2) Something like this maybe, does it work for you?
Created attachment 35870 [details] mod_xml2enc: pass all meta buckets down the chain (v3) Well, better if it compiles...
Created attachment 35871 [details] mod_xml2enc: pass all meta buckets down the chain (v4) ... and yet better without other work (or not) in progress. Sorry, I need a pause :)
Created attachment 35872 [details] mod_xml2enc: pass all meta buckets down the chain (v5) An APR_BRIGADE_CONCAT() added for safety (unlikely case where EOS is not the latest bucket).
v5 committed in r1829038, will let it linger a bit for review before proposing a backport to 2.4.x.
+ r1829039.
Yann, thanks for jumping on this so fast. I got something to test... :) One comment about the current code in trunk (SVN r1829039), file modules/filters/mod_xml2enc.c line 550. Shouldn't pending_metadata get reset to zero within the if block?
(In reply to Micha Lenk from comment #13) > Yann, thanks for jumping on this so fast. I got something to test... :) You're welcome. Fortunately you didn't test the series one by one :) > > One comment about the current code in trunk (SVN r1829039), file > modules/filters/mod_xml2enc.c line 550. Shouldn't pending_metadata get reset > to zero within the if block? It's a local variable and the filter is leaving at this point.
>> Shouldn't pending_metadata get reset to zero within the if block? > > It's a local variable and the filter is leaving at this point. Right. Thanks for the explanation.
I've just tested the cherry-picked SVN revisions r1829038 and r1829039 applied to a httpd-2.4.25 build and can confirm that this variant of the patch also fixes our issue. Thanks for considering to backport this.
Thanks for testing, backport proposed in r1830524.
> Thanks for testing, backport proposed in r1830524. The above mentioned change of the STATUS document lists another SVN revision (r1830523). So I re-run our tests with that one applied on top. The issue remains fixed. Thanks!
Thanks Mika, it was more or less a cosmetic change which allowed me to close the corresponding pull request on github (with the commit message, I missed that on the first commits) :p
backported in 2.4.x in r1834104 This is part of 2.4.34