On Thu, Dec 12, 2013 at 7:14 PM, Yann Ylavic <[email protected]> wrote:
> On Thu, Dec 12, 2013 at 6:45 PM, Yann Ylavic <[email protected]> wrote:
>
>> Here is a proposal (patch against ap_proxy_http_process_response) to
>> address the double lifetime transformation of the buckets from the backend
>> when its connection is released early (on EOS, before the last buckets are
>> forwarded to the client).
>>
>
> Maybe this version is more safe should bb be (later) created with a
> different pool/bucket_alloc than pass_bb :
>
> [...]
>
> - for (e = APR_BRIGADE_FIRST(pass_bb); e
> - != APR_BRIGADE_SENTINEL(pass_bb); e
> - = APR_BUCKET_NEXT(e)) {
> - apr_bucket_setaside(e, r->pool);
> + rv = ap_save_brigade(NULL, &pass_bb, &bb,
> r->pool);
> + if (rv != APR_SUCCESS) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r,
> APLOGNO()
> + "Failed to save final output
> brigade");
> + return HTTP_INTERNAL_SERVER_ERROR;
>
> }
>
This seems not a good idea to return an error here, so this patch is no
good either. Let me try a third...
Since the original code did not check the return value, I assume the patch
does not have to :
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1550532)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1729,12 +1729,8 @@ int ap_proxy_http_process_response(apr_pool_t * p,
break;
}
- /* Switch the allocator lifetime of the buckets */
- proxy_buckets_lifetime_transform(r, bb, pass_bb);
-
/* found the last brigade? */
- if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(pass_bb))) {
-
+ if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
/* signal that we must leave */
finish = TRUE;
@@ -1742,19 +1738,12 @@ int ap_proxy_http_process_response(apr_pool_t * p,
* data that lives only as long as the backend
connection.
* Force a setaside so these transient buckets
become heap
* buckets that live as long as the request.
+ * Calling ap_save_brigade with NULL as filter is
OK,
+ * because pass_bb brigade already has been
created and
+ * does not need to get created by ap_save_brigade.
*/
- for (e = APR_BRIGADE_FIRST(pass_bb); e
- != APR_BRIGADE_SENTINEL(pass_bb); e
- = APR_BUCKET_NEXT(e)) {
- apr_bucket_setaside(e, r->pool);
- }
+ ap_save_brigade(NULL, &pass_bb, &bb, r->pool);
- /* finally it is safe to clean up the brigade from
the
- * connection pool, as we have forced a setaside
on all
- * buckets.
- */
- apr_brigade_cleanup(bb);
-
/* make sure we release the backend connection as
soon
* as we know we are done, so that the backend
isn't
* left waiting for a slow client to eventually
@@ -1764,8 +1753,11 @@ int ap_proxy_http_process_response(apr_pool_t * p,
backend, r->server);
/* Ensure that the backend is not reused */
*backend_ptr = NULL;
-
}
+ else {
+ /* Switch the allocator lifetime of the buckets */
+ proxy_buckets_lifetime_transform(r, bb, pass_bb);
+ }
/* try send what we read */
if (ap_pass_brigade(r->output_filters, pass_bb) !=
APR_SUCCESS
[EOS]
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1550532)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -1729,12 +1729,8 @@ int ap_proxy_http_process_response(apr_pool_t * p,
break;
}
- /* Switch the allocator lifetime of the buckets */
- proxy_buckets_lifetime_transform(r, bb, pass_bb);
-
/* found the last brigade? */
- if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(pass_bb))) {
-
+ if (APR_BUCKET_IS_EOS(APR_BRIGADE_LAST(bb))) {
/* signal that we must leave */
finish = TRUE;
@@ -1742,19 +1738,12 @@ int ap_proxy_http_process_response(apr_pool_t * p,
* data that lives only as long as the backend connection.
* Force a setaside so these transient buckets become heap
* buckets that live as long as the request.
+ * Calling ap_save_brigade with NULL as filter is OK,
+ * because pass_bb brigade already has been created and
+ * does not need to get created by ap_save_brigade.
*/
- for (e = APR_BRIGADE_FIRST(pass_bb); e
- != APR_BRIGADE_SENTINEL(pass_bb); e
- = APR_BUCKET_NEXT(e)) {
- apr_bucket_setaside(e, r->pool);
- }
+ ap_save_brigade(NULL, &pass_bb, &bb, r->pool);
- /* finally it is safe to clean up the brigade from the
- * connection pool, as we have forced a setaside on all
- * buckets.
- */
- apr_brigade_cleanup(bb);
-
/* make sure we release the backend connection as soon
* as we know we are done, so that the backend isn't
* left waiting for a slow client to eventually
@@ -1764,8 +1753,11 @@ int ap_proxy_http_process_response(apr_pool_t * p,
backend, r->server);
/* Ensure that the backend is not reused */
*backend_ptr = NULL;
-
}
+ else {
+ /* Switch the allocator lifetime of the buckets */
+ proxy_buckets_lifetime_transform(r, bb, pass_bb);
+ }
/* try send what we read */
if (ap_pass_brigade(r->output_filters, pass_bb) != APR_SUCCESS
