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 :
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1550128)
+++ 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,17 @@ 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);
+ 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;
}
- /* 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 +1758,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 1550128)
+++ 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,17 @@ 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);
+ 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;
}
- /* 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 +1758,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
