On 12/13/2016 02:49 PM, Yann Ylavic wrote: > On Tue, Dec 13, 2016 at 10:48 AM, Plüm, Rüdiger, Vodafone Group > <[email protected]> wrote:
>> Another aspect of all these patches that I don't get is why we need >> to eat the contents of the original brigade? IMHO we don't need to do >> this. It is thrown away automatically at the end of the request and >> as we leave with an AP_FILTER_ERROR after the ap_die it should never >> be sent. > > The pros with this is indeed the reduced complexity (though the loop > to walk the brigade already existed), the cons are that we rely on the > caller/handler to not ignore ap_pass_brigade()'s return value and IMHO this would violate a principal and easy to follow design pattern. I don't think that we can and want to protect module developers from all errors they could make, especially if they are easy to spot and understand. And if why catch this here? If someone in the the filter chain continues sending content even after passing things down the chain caused an error weird things can happen before us or if the source is after us after us. This change is also unrelated to the bad header issue and I think if there is interest to address this it should be done in a separate patch set. Furthermore it causes a handler to continue generating content in a properly resource intensive way that we drop directly. And the handler has no chance to know this because we return APR_SUCCESS. > mainly that we could close/reset an incomplete backend/origin > connection (hence not reusable) while it may not be the culprit (e.g., > when some "Header set ..." is). > >> If we want to play safe we can remove ourselves from the >> filterchain after returning from ap_die. This could make the whole >> stuff much less complex. > > Still we'd depend on the caller to do the right thing with errors > (AP_FILTER_ERROR). I see this differently. See above. > I don't find the change too complex after all, and that's a quite > critical filter for doing the right/safe thing... > > I'm even inclined to do the below changes, so that we are really safe > (i.e. ignore any data) after EOS... After this patch we do the wrong thing with an EOC bucket. The current contract is that an EOC bucket *anywhere* in the brigade causes the header filter to go out of the way. After the patch this is broken if a bad header is present. But as EOC tells us to get out of the way and do nothing, we don't need to take care about bad headers. This breaks edge case with mod_proxy_http Albeit this would be fixable I see more negative points then positive points here and I am -0.5 on that content slurping. Regards Rüdiger
