You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by rb...@covalent.net on 2001/01/27 00:00:59 UTC
brigade patch, last time.
This patch solves multiple problems.
1) buffering brigade output.
2) ap_r* uses the buffered brigade functions.
3) implements a group of ap_f* functions which are the general case of
ap_r*
4) Solves the http_header filter problem, by using the general brigade
functions.
The only reason for not using this patch that I can see, is that it is
possible to get data out of order. However, with some diligence, that is
easy to avoid. The only way this will be an issue, is if somebody decides
they want to use the bucket functions directly. If they are doing that,
then they will need to use the apr_brigade_flush function first.
Can people comment on this, so that I can commit it?
Ryan
Index: include/http_protocol.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/http_protocol.h,v
retrieving revision 1.46
diff -u -d -b -w -u -r1.46 http_protocol.h
--- include/http_protocol.h 2001/01/24 02:14:23 1.46
+++ include/http_protocol.h 2001/01/26 20:09:38
@@ -62,6 +62,7 @@
#include "apr_hooks.h"
#include "apr_portable.h"
#include "apr_mmap.h"
+#include "apr_buckets.h"
#ifdef __cplusplus
extern "C" {
@@ -86,13 +87,13 @@
/**
* Send the minimal part of an HTTP response header.
* @param r The current request
- * @param buf The buffer to add the header to.
+ * @param bb The brigade to add the header to.
* @warning Modules should be very careful about using this, and should
* prefer ap_send_http_header(). Much of the HTTP/1.1 implementation
* correctness depends on code in ap_send_http_header().
- * @deffunc void ap_basic_http_header(request_rec *r, char *buf)
+ * @deffunc void ap_basic_http_header(request_rec *r, apr_bucket_brigade *bb)
*/
-AP_DECLARE(void) ap_basic_http_header(request_rec *r, char *buf);
+AP_DECLARE(void) ap_basic_http_header(request_rec *r, apr_bucket_brigade *bb);
/**
* Send the Status-Line and header fields for HTTP response
@@ -308,7 +309,11 @@
* @return The number of bytes sent
* @deffunc int ap_rputc(int c, request_rec *r)
*/
-AP_DECLARE(int) ap_rputc(int c, request_rec *r);
+#define ap_rputc(c, r) \
+ if (!r->bb) { \
+ r->bb = apr_brigade_create(r->pool); \
+ } \
+ ap_fputc(r->output_filters, r->bb, c)
/**
* Output a string for the current request
* @param str The string to output
@@ -316,7 +321,11 @@
* @return The number of bytes sent
* @deffunc int ap_rputs(const char *str, request_rec *r)
*/
-AP_DECLARE(int) ap_rputs(const char *str, request_rec *r);
+#define ap_rputs(str, r) \
+ if (!r->bb) { \
+ r->bb = apr_brigade_create(r->pool); \
+ } \
+ ap_fputs(r->output_filters, r->bb, str)
/**
* Write a buffer for the current request
* @param buf The buffer to write
@@ -325,7 +334,11 @@
* @return The number of bytes sent
* @deffunc int ap_rwrite(const void *buf, int nbyte, request_rec *r)
*/
-AP_DECLARE(int) ap_rwrite(const void *buf, int nbyte, request_rec *r);
+#define ap_rwrite(str, n, r) \
+ if (!r->bb) { \
+ r->bb = apr_brigade_create(r->pool); \
+ } \
+ ap_fwrite(r->output_filters, r->bb, str, n)
/**
* Write an unspecified number of strings to the request
* @param r The current request
@@ -333,7 +346,11 @@
* @return The number of bytes sent
* @deffunc int ap_rvputs(request_rec *r, ...)
*/
-AP_DECLARE_NONSTD(int) ap_rvputs(request_rec *r,...);
+#define ap_rvputs(r, args...) \
+ if (!r->bb) { \
+ r->bb = apr_brigade_create(r->pool); \
+ } \
+ ap_fvputs(r->output_filters, r->bb, ##args)
/**
* Output data to the client in a printf format
* @param r The current request
@@ -342,7 +359,11 @@
* @return The number of bytes sent
* @deffunc int ap_vrprintf(request_rec *r, const char *fmt, va_list vlist)
*/
-AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list vlist);
+#define ap_vrprintf(r, fmt, vlist) \
+ if (!r->bb) { \
+ r->bb = apr_brigade_create(r->pool); \
+ } \
+ ap_vfprintf(r->output_filters, r->bb, fmt, vlist)
/**
* Output data to the client in a printf format
* @param r The current request
@@ -351,15 +372,22 @@
* @return The number of bytes sent
* @deffunc int ap_rprintf(request_rec *r, const char *fmt, ...)
*/
-AP_DECLARE_NONSTD(int) ap_rprintf(request_rec *r, const char *fmt,...)
- __attribute__((format(printf,2,3)));
+#define ap_rprintf(r, fmt, args...) \
+ if (!r->bb) { \
+ r->bb = apr_brigade_create(r->pool); \
+ } \
+ ap_fprintf(r->output_filters, r->bb, fmt, ##args)
/**
* Flush all of the data for the current request to the client
* @param r The current request
* @return The number of bytes sent
* @deffunc int ap_rflush(request_rec *r)
*/
-AP_DECLARE(int) ap_rflush(request_rec *r);
+#define ap_rflush(r) \
+ if (!r->bb) { \
+ r->bb = apr_brigade_create(r->pool); \
+ } \
+ ap_fflush(r->output_filters, r->bb)
/**
* Index used in custom_responses array for a specific error code
Index: include/httpd.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/httpd.h,v
retrieving revision 1.131
diff -u -d -b -w -u -r1.131 httpd.h
--- include/httpd.h 2001/01/20 06:05:14 1.131
+++ include/httpd.h 2001/01/26 20:09:38
@@ -85,6 +85,7 @@
#include "apr_pools.h"
#include "apr_time.h"
#include "apr_network_io.h"
+#include "apr_buckets.h"
#ifdef CORE_PRIVATE
@@ -809,6 +810,8 @@
/** A flag to determine if the eos bucket has been sent yet
* @defvar int eos_sent */
int eos_sent;
+
+ apr_bucket_brigade *bb;
/* Things placed at the end of the record to avoid breaking binary
* compatibility. It would be nice to remember to reorder the entire
Index: include/util_filter.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/util_filter.h,v
retrieving revision 1.36
diff -u -d -b -w -u -r1.36 util_filter.h
--- include/util_filter.h 2001/01/22 21:57:56 1.36
+++ include/util_filter.h 2001/01/26 20:09:38
@@ -403,6 +403,16 @@
AP_DECLARE(apr_status_t) ap_save_brigade(ap_filter_t *f, apr_bucket_brigade **save_to,
apr_bucket_brigade **b);
+AP_DECLARE(apr_status_t) ap_fwrite(ap_filter_t *f, apr_bucket_brigade *bb,
+ const char *str, apr_size_t nbyte);
+AP_DECLARE(apr_status_t) ap_fputs(ap_filter_t *f, apr_bucket_brigade *bb,
+ const char *str);
+AP_DECLARE(apr_status_t) ap_fputc(ap_filter_t *f, apr_bucket_brigade *bb,
+ char str);
+AP_DECLARE(apr_status_t) ap_fvputs(ap_filter_t *f, apr_bucket_brigade *bb, ...);
+AP_DECLARE(apr_status_t) ap_fprintf(ap_filter_t *f, apr_bucket_brigade *bb, const char *fmt, ...);
+
+
#ifdef __cplusplus
}
#endif
Index: modules/http/http_core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_core.c,v
retrieving revision 1.250
diff -u -d -b -w -u -r1.250 http_core.c
--- modules/http/http_core.c 2001/01/25 02:51:30 1.250
+++ modules/http/http_core.c 2001/01/26 20:09:53
@@ -3590,8 +3590,6 @@
AP_FTYPE_CONTENT);
ap_register_output_filter("CHUNK", chunk_filter, AP_FTYPE_TRANSCODE);
ap_register_output_filter("COALESCE", coalesce_filter, AP_FTYPE_CONTENT);
- ap_register_output_filter("OLD_WRITE", ap_old_write_filter,
- AP_FTYPE_CONTENT - 1);
}
AP_DECLARE_DATA module core_module = {
Index: modules/http/http_protocol.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.281
diff -u -d -b -w -u -r1.281 http_protocol.c
--- modules/http/http_protocol.c 2001/01/26 17:57:55 1.281
+++ modules/http/http_protocol.c 2001/01/26 20:09:56
@@ -1572,13 +1572,18 @@
static void end_output_stream(request_rec *r)
{
- apr_bucket_brigade *bb;
apr_bucket *b;
- bb = apr_brigade_create(r->pool);
+ if (r->bb) {
+ apr_brigade_flush(r->bb);
+ }
+ else {
+ r->bb = apr_brigade_create(r->pool);
+ }
+
b = apr_bucket_create_eos();
- APR_BRIGADE_INSERT_TAIL(bb, b);
- ap_pass_brigade(r->output_filters, bb);
+ APR_BRIGADE_INSERT_TAIL(r->bb, b);
+ ap_pass_brigade(r->output_filters, r->bb);
}
void ap_finalize_sub_req_protocol(request_rec *sub)
@@ -1777,7 +1782,7 @@
typedef struct header_struct {
request_rec *r;
- char *buf;
+ apr_bucket_brigade *bb;
} header_struct;
/* Send a single HTTP header field to the client. Note that this function
@@ -1792,8 +1797,7 @@
headfield = apr_pstrcat(h->r->pool, fieldname, ": ", fieldval, CRLF, NULL);
ap_xlate_proto_to_ascii(headfield, strlen(headfield));
- apr_cpystrn(h->buf, headfield, strlen(headfield) + 1);
- h->buf += strlen(headfield);
+ apr_brigade_write(h->bb, headfield, strlen(headfield) + 1);
return 1;
}
@@ -1805,7 +1809,7 @@
return 1;
}
-AP_DECLARE(void) ap_basic_http_header(request_rec *r, char *buf)
+AP_DECLARE(void) ap_basic_http_header(request_rec *r, apr_bucket_brigade *bb)
{
char *protocol;
char *date = NULL;
@@ -1836,14 +1840,13 @@
tmp = apr_pstrcat(r->pool, protocol, " ", r->status_line, CRLF, NULL);
ap_xlate_proto_to_ascii(tmp, strlen(tmp));
- apr_cpystrn(buf, tmp, strlen(tmp) + 1);
- buf += strlen(tmp);
+ apr_brigade_write(bb, tmp, strlen(tmp) + 1);
date = apr_palloc(r->pool, APR_RFC822_DATE_LEN);
apr_rfc822_date(date, r->request_time);
h.r = r;
- h.buf = buf;
+ h.bb = bb;
form_header_field(&h, "Date", date);
form_header_field(&h, "Server", ap_get_server_version());
@@ -1868,18 +1871,18 @@
* It is more expensive to check the User-Agent than it is to just add the
* bytes, so we haven't used the BrowserMatch feature here.
*/
-static void terminate_header(char *buf)
+static void terminate_header(apr_bucket_brigade *bb)
{
- int len = strlen(buf);
- char *headfield = buf + len;
+ int len = strlen(bb->start);
char *tmp = "X-Pad: avoid browser bug" CRLF;
+ char *crlf = CRLF;
if (len >= 255 && len <= 257) {
- apr_cpystrn(headfield, tmp, strlen(tmp) + 1);
- headfield += strlen(tmp);
+ ap_xlate_proto_to_ascii(tmp, strlen(tmp));
+ apr_brigade_write(bb, tmp, strlen(tmp) + 1);
}
- apr_cpystrn(headfield, CRLF, strlen(CRLF) + 1);
- ap_xlate_proto_to_ascii(buf + len, strlen(buf + len));
+ ap_xlate_proto_to_ascii(crlf, strlen(crlf));
+ apr_brigade_write(bb, crlf, strlen(crlf) + 1);
}
/*
@@ -2125,42 +2128,28 @@
int ap_send_http_options(request_rec *r)
{
- char *buff;
- apr_bucket *b;
- apr_bucket_brigade *bb;
- apr_size_t len = 0;
+ apr_bucket_brigade *bb = apr_brigade_create(r->pool);
header_struct h;
if (r->assbackwards)
return DECLINED;
- apr_table_do((int (*) (void *, const char *, const char *)) compute_header_len,
- (void *) &len, r->headers_out, NULL);
-
- /* Need to add a fudge factor so that the CRLF at the end of the headers
- * and the basic http headers don't overflow this buffer.
- */
- len += strlen(ap_get_server_version()) + 100;
- buff = apr_pcalloc(r->pool, len);
- ap_basic_http_header(r, buff);
+ ap_basic_http_header(r, bb);
apr_table_setn(r->headers_out, "Content-Length", "0");
apr_table_setn(r->headers_out, "Allow", make_allow(r));
ap_set_keepalive(r);
h.r = r;
- h.buf = buff;
+ h.bb = bb;
apr_table_do((int (*) (void *, const char *, const char *)) form_header_field,
(void *) &h, r->headers_out, NULL);
- terminate_header(buff);
+ terminate_header(bb);
r->bytes_sent = 0;
- bb = apr_brigade_create(r->pool);
- b = apr_bucket_create_pool(buff, strlen(buff), r->pool);
- APR_BRIGADE_INSERT_TAIL(bb, b);
ap_pass_brigade(r->output_filters, bb);
return OK;
@@ -2434,8 +2423,7 @@
char *date = NULL;
request_rec *r = f->r;
char *buff, *buff_start;
- apr_bucket *e;
- apr_bucket_brigade *b2;
+ apr_bucket_brigade *b2 = apr_brigade_create(r->pool);
apr_size_t len = 0;
header_struct h;
header_filter_ctx *ctx = f->ctx;
@@ -2486,11 +2474,11 @@
*/
len += strlen(ap_get_server_version()) + 100;
buff_start = buff = apr_pcalloc(r->pool, len);
- ap_basic_http_header(r, buff);
+ ap_basic_http_header(r, b2);
buff += strlen(buff);
h.r = r;
- h.buf = buff;
+ h.bb = b2;
ap_set_keepalive(r);
@@ -2587,13 +2575,10 @@
(void *) &h, r->headers_out, NULL);
}
- terminate_header(buff);
+ terminate_header(b2);
r->sent_bodyct = 1; /* Whatever follows is real body stuff... */
- b2 = apr_brigade_create(r->pool);
- e = apr_bucket_create_pool(buff_start, strlen(buff_start), r->pool);
- APR_BRIGADE_INSERT_HEAD(b2, e);
ap_pass_brigade(f->next, b2);
if (r->header_only) {
@@ -2944,244 +2929,6 @@
return mm->size; /* XXX - change API to report apr_status_t? */
}
#endif /* APR_HAS_MMAP */
-
-typedef struct {
- char *buf;
- char *cur;
- apr_size_t avail;
-} old_write_filter_ctx;
-
-AP_CORE_DECLARE_NONSTD(apr_status_t) ap_old_write_filter(
- ap_filter_t *f, apr_bucket_brigade *bb)
-{
- old_write_filter_ctx *ctx = f->ctx;
-
- AP_DEBUG_ASSERT(ctx);
-
- if (ctx->buf != NULL) {
- apr_size_t nbyte = ctx->cur - ctx->buf;
-
- if (nbyte != 0) {
- /* whatever is coming down the pipe (we don't care), we
- can simply insert our buffered data at the front and
- pass the whole bundle down the chain. */
- apr_bucket *b = apr_bucket_create_heap(ctx->buf, nbyte, 0, NULL);
- APR_BRIGADE_INSERT_HEAD(bb, b);
- ctx->buf = NULL;
- }
- }
-
- return ap_pass_brigade(f->next, bb);
-}
-
-static apr_status_t flush_buffer(request_rec *r, old_write_filter_ctx *ctx,
- const char *extra, apr_size_t extra_len)
-{
- apr_bucket_brigade *bb = apr_brigade_create(r->pool);
- apr_size_t nbyte = ctx->cur - ctx->buf;
- apr_bucket *b = apr_bucket_create_heap(ctx->buf, nbyte, 0, NULL);
-
- APR_BRIGADE_INSERT_TAIL(bb, b);
- ctx->buf = NULL;
-
- /* if there is extra data, then send that, too */
- if (extra != NULL) {
- b = apr_bucket_create_transient(extra, extra_len);
- APR_BRIGADE_INSERT_TAIL(bb, b);
- }
-
- return ap_pass_brigade(r->output_filters, bb);
-}
-
-static apr_status_t buffer_output(request_rec *r,
- const char *str, apr_size_t len)
-{
- ap_filter_t *f;
- old_write_filter_ctx *ctx;
- apr_size_t amt;
- apr_status_t status;
-
- if (len == 0)
- return APR_SUCCESS;
-
- /* ### future optimization: record some flags in the request_rec to
- ### say whether we've added our filter, and whether it is first. */
-
- /* this will typically exit on the first test */
- for (f = r->output_filters; f != NULL; f = f->next)
- if (strcmp("OLD_WRITE", f->frec->name) == 0)
- break;
- if (f == NULL) {
- /* our filter hasn't been added yet */
- ctx = apr_pcalloc(r->pool, sizeof(*ctx));
- ap_add_output_filter("OLD_WRITE", ctx, r, r->connection);
- }
-
- /* if the first filter is not our buffering filter, then we have to
- deliver the content through the normal filter chain */
- if (strcmp("OLD_WRITE", r->output_filters->frec->name) != 0) {
- apr_bucket_brigade *bb = apr_brigade_create(r->pool);
- apr_bucket *b = apr_bucket_create_transient(str, len);
- APR_BRIGADE_INSERT_TAIL(bb, b);
-
- return ap_pass_brigade(r->output_filters, bb);
- }
-
- /* grab the context from our filter */
- ctx = r->output_filters->ctx;
-
- /* if there isn't a buffer in the context yet, put one there. */
- if (ctx->buf == NULL) {
- /* use the heap so it will get free'd after being flushed */
- ctx->avail = AP_MIN_BYTES_TO_WRITE;
- ctx->buf = ctx->cur = malloc(ctx->avail);
- }
-
- /* squeeze the data into the existing buffer */
- if (len <= ctx->avail) {
- memcpy(ctx->cur, str, len);
- ctx->cur += len;
- if ((ctx->avail -= len) == 0)
- return flush_buffer(r, ctx, NULL, 0);
- return APR_SUCCESS;
- }
-
- /* the new content can't fit in the existing buffer */
-
- if (len >= AP_MIN_BYTES_TO_WRITE) {
- /* it is really big. send what we have, and the new stuff. */
- return flush_buffer(r, ctx, str, len);
- }
-
- /* the new data is small. put some into the current buffer, flush it,
- and then drop the remaining into a new buffer. */
- amt = ctx->avail;
- memcpy(ctx->cur, str, amt);
- ctx->cur += amt;
- ctx->avail = 0;
- if ((status = flush_buffer(r, ctx, NULL, 0)) != APR_SUCCESS)
- return status;
-
- ctx->buf = malloc(AP_MIN_BYTES_TO_WRITE);
- memcpy(ctx->buf, str + amt, len - amt);
- ctx->cur = ctx->buf + (len - amt);
- ctx->avail = AP_MIN_BYTES_TO_WRITE - (len - amt);
-
- return APR_SUCCESS;
-}
-
-AP_DECLARE(int) ap_rputc(int c, request_rec *r)
-{
- char c2 = (char)c;
-
- if (r->connection->aborted) {
- return -1;
- }
-
- if (buffer_output(r, &c2, 1) != APR_SUCCESS)
- return -1;
-
- return c;
-}
-
-AP_DECLARE(int) ap_rputs(const char *str, request_rec *r)
-{
- apr_size_t len;
-
- if (r->connection->aborted)
- return -1;
-
- if (buffer_output(r, str, len = strlen(str)) != APR_SUCCESS)
- return -1;
-
- return len;
-}
-
-AP_DECLARE(int) ap_rwrite(const void *buf, int nbyte, request_rec *r)
-{
- if (r->connection->aborted)
- return -1;
-
- if (buffer_output(r, buf, nbyte) != APR_SUCCESS)
- return -1;
-
- return nbyte;
-}
-
-AP_DECLARE(int) ap_vrprintf(request_rec *r, const char *fmt, va_list va)
-{
- char buf[4096];
- apr_size_t written;
-
- if (r->connection->aborted)
- return -1;
-
- /* ### fix this mechanism to allow more than 4K of output */
- written = apr_vsnprintf(buf, sizeof(buf), fmt, va);
- if (buffer_output(r, buf, written) != APR_SUCCESS)
- return -1;
-
- return written;
-}
-
-AP_DECLARE_NONSTD(int) ap_rprintf(request_rec *r, const char *fmt, ...)
-{
- va_list va;
- int n;
-
- if (r->connection->aborted)
- return -1;
-
- va_start(va, fmt);
- n = ap_vrprintf(r, fmt, va);
- va_end(va);
-
- return n;
-}
-
-AP_DECLARE_NONSTD(int) ap_rvputs(request_rec *r, ...)
-{
- va_list va;
- const char *s;
- apr_size_t len;
- apr_size_t written = 0;
-
- if (r->connection->aborted)
- return -1;
-
- /* ### TODO: if the total output is large, put all the strings
- ### into a single brigade, rather than flushing each time we
- ### fill the buffer */
- va_start(va, r);
- while (1) {
- s = va_arg(va, const char *);
- if (s == NULL)
- break;
-
- len = strlen(s);
- if (buffer_output(r, s, len) != APR_SUCCESS) {
- return -1;
- }
-
- written += len;
- }
- va_end(va);
-
- return written;
-}
-
-AP_DECLARE(int) ap_rflush(request_rec *r)
-{
- apr_bucket_brigade *bb;
- apr_bucket *b;
-
- bb = apr_brigade_create(r->pool);
- b = apr_bucket_create_flush();
- APR_BRIGADE_INSERT_TAIL(bb, b);
- if (ap_pass_brigade(r->output_filters, bb) != APR_SUCCESS)
- return -1;
- return 0;
-}
static const char *add_optional_notes(request_rec *r,
const char *prefix,
Index: server/util_filter.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/util_filter.c,v
retrieving revision 1.43
diff -u -d -b -w -u -r1.43 util_filter.c
--- server/util_filter.c 2001/01/19 07:04:29 1.43
+++ server/util_filter.c 2001/01/26 20:10:17
@@ -265,3 +265,59 @@
APR_BRIGADE_CONCAT(*saveto, *b);
return APR_SUCCESS;
}
+
+AP_DECLARE(apr_status_t) ap_fwrite(ap_filter_t *f, apr_bucket_brigade *bb,
+ const char *str, apr_size_t nbyte)
+{
+ apr_bucket *b;
+ apr_status_t status;
+
+ if (nbyte == 0)
+ return 0;
+
+ b = APR_BRIGADE_LAST(bb);
+
+ status = apr_brigade_write(bb, str, nbyte);
+
+ /* We are just checking to see if we just converted the
+ * buffer into a new bucket and put it at the end of the brigade. If
+ * we did, we want to pass the brigade to the next filter. If not,
+ * we just keep going. This allows us to use the network to limit how
+ * much data we send at any one time.
+ */
+ if (b != APR_BRIGADE_LAST(bb)) {
+ ap_pass_brigade(f->next, bb);
+ }
+ return status;
+}
+
+AP_DECLARE(apr_status_t) ap_fputs(ap_filter_t *f, apr_bucket_brigade *bb,
+ const char *str)
+{
+ int j = strlen(str);
+
+ return ap_fwrite(f, bb, str, j);
+}
+
+AP_DECLARE(apr_status_t) ap_fputc(ap_filter_t *f, apr_bucket_brigade *bb,
+ char str)
+{
+ return ap_fwrite(f, bb, &str, 1);
+}
+
+AP_DECLARE(apr_status_t) ap_fvputs(ap_filter_t *f, apr_bucket_brigade *bb, ...)
+{
+ apr_size_t written;
+ va_list va;
+
+ va_start(va, bb);
+ written = apr_brigade_vputstrs(bb, va);
+ va_end(va);
+ return written;
+}
+
+AP_DECLARE(apr_status_t) ap_fprintf(ap_filter_t *f, apr_bucket_brigade *bb, const char *fmt, ...)
+{
+
+ return 1;
+}
Index: srclib/apr-util/buckets/apr_brigade.c
===================================================================
RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.3
diff -u -d -b -w -u -r1.3 apr_brigade.c
--- srclib/apr-util/buckets/apr_brigade.c 2001/01/19 07:01:59 1.3
+++ srclib/apr-util/buckets/apr_brigade.c 2001/01/26 20:11:12
@@ -99,6 +99,9 @@
b = apr_palloc(p, sizeof(*b));
b->p = p;
+
+ b->start = b->end = NULL;
+
APR_RING_INIT(&b->list, apr_bucket, link);
apr_register_cleanup(b->p, b, apr_brigade_cleanup, apr_brigade_cleanup);
@@ -185,12 +188,43 @@
return vec - orig;
}
+static int check_brigade_flush(char *start, char **end, const char **str,
+ apr_size_t *n, apr_bucket_brigade *b)
+{
+ if (*n > (APR_BUCKET_BUFF_SIZE - (*end - start))) {
+ if (start) {
+ int size = APR_BUCKET_BUFF_SIZE - (*end - start);
+ memcpy(*end, *str, size);
+ *n -= size;
+ *end += size;
+ *str += size;
+ apr_brigade_flush(b);
+ }
+ if (*n > APR_BUCKET_BUFF_SIZE) {
+ apr_bucket *e = apr_bucket_create_heap(*str, *n, 1, NULL);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ return 1;
+ }
+ }
+ return 0;
+}
+
+APU_DECLARE(void) apr_brigade_flush(apr_bucket_brigade *b)
+{
+ apr_bucket *e;
+ apr_size_t i = b->end - b->start;
+
+ if (i) {
+ e = apr_bucket_create_heap(b->start, i, 0, NULL);
+ APR_BRIGADE_INSERT_TAIL(b, e);
+ b->start = b->end = NULL;
+ }
+}
+
APU_DECLARE(int) apr_brigade_vputstrs(apr_bucket_brigade *b, va_list va)
{
- apr_bucket *r;
const char *x;
int j, k;
- apr_size_t i;
for (k = 0;;) {
x = va_arg(va, const char *);
@@ -198,18 +232,51 @@
break;
j = strlen(x);
- /* XXX: copy or not? let the caller decide? */
- r = apr_bucket_create_heap(x, j, 1, &i);
- if (i != j) {
- /* Do we need better error reporting? */
- return -1;
+ k += apr_brigade_write(b, x, j);
}
- k += i;
+ return k;
+}
- APR_BRIGADE_INSERT_TAIL(b, r);
+APU_DECLARE(int) apr_brigade_putc(apr_bucket_brigade *b, const char c)
+{
+ apr_size_t nbyte = 1;
+ const char *str = &c;
+
+ if (check_brigade_flush(b->start, &b->end, &str, &nbyte, b)) {
+ return 1;
}
- return k;
+ if (!b->start) {
+ b->start = b->end = malloc(APR_BUCKET_BUFF_SIZE);
+ }
+
+ memcpy(b->end, &c, 1);
+ b->end++;
+
+ return 1;
+}
+
+APU_DECLARE(int) apr_brigade_write(apr_bucket_brigade *b, const char *str, apr_size_t nbyte)
+{
+ apr_size_t n = nbyte;
+
+ if (check_brigade_flush(b->start, &b->end, &str, &nbyte, b)) {
+ return n;
+ }
+
+ if (!b->start) {
+ b->start = b->end = malloc(APR_BUCKET_BUFF_SIZE);
+ }
+
+ memcpy(b->end, str, nbyte);
+ b->end += nbyte;
+
+ return n;
+}
+
+APU_DECLARE(int) apr_brigade_puts(apr_bucket_brigade *b, const char *str)
+{
+ return apr_brigade_write(b, str, strlen(str));
}
APU_DECLARE_NONSTD(int) apr_brigade_putstrs(apr_bucket_brigade *b, ...)
@@ -240,13 +307,9 @@
* directly into a bucket. I'm being lazy right now. RBB
*/
char buf[4096];
- apr_bucket *r;
- int res;
-
- res = apr_vsnprintf(buf, 4096, fmt, va);
- r = apr_bucket_create_heap(buf, strlen(buf), 1, NULL);
- APR_BRIGADE_INSERT_TAIL(b, r);
+ apr_vsnprintf(buf, 4096, fmt, va);
- return res;
+ return apr_brigade_puts(b, buf);
}
+
Index: srclib/apr-util/include/apr_buckets.h
===================================================================
RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
retrieving revision 1.67
diff -u -d -b -w -u -r1.67 apr_buckets.h
--- srclib/apr-util/include/apr_buckets.h 2001/01/23 22:24:06 1.67
+++ srclib/apr-util/include/apr_buckets.h 2001/01/26 20:11:16
@@ -78,6 +78,8 @@
* @package Bucket Brigades
*/
+#define APR_BUCKET_BUFF_SIZE 9000
+
typedef enum {APR_BLOCK_READ, APR_NONBLOCK_READ} apr_read_type_e;
/*
@@ -237,6 +239,8 @@
* the destroying function is responsible for killing the cleanup.
*/
apr_pool_t *p;
+ char *start;
+ char *end;
/** The buckets in the brigade are on this list. */
/*
* XXX: the apr_bucket_list structure doesn't actually need a name tag
@@ -628,9 +632,10 @@
APU_DECLARE(int) apr_brigade_to_iovec(apr_bucket_brigade *b,
struct iovec *vec, int nvec);
+APU_DECLARE(void) apr_brigade_flush(apr_bucket_brigade *b);
+
/**
- * This function writes a list of strings into a bucket brigade. We just
- * allocate a new heap bucket for each string.
+ * This function writes a list of strings into a bucket brigade.
* @param b The bucket brigade to add to
* @param va A list of strings to add
* @return The number of bytes added to the brigade
@@ -639,8 +644,34 @@
APU_DECLARE(int) apr_brigade_vputstrs(apr_bucket_brigade *b, va_list va);
/**
+ * This function writes an string into a bucket brigade.
+ * @param b The bucket brigade to add to
+ * @param str The string to add
+ * @return The number of bytes added to the brigade
+ * @deffunc int apr_brigade_write(ap_bucket_brigade *b, const char *str)
+ */
+APU_DECLARE(int) apr_brigade_write(apr_bucket_brigade *b, const char *str, apr_size_t nbyte);
+
+/**
+ * This function writes an string into a bucket brigade.
+ * @param b The bucket brigade to add to
+ * @param str The string to add
+ * @return The number of bytes added to the brigade
+ * @deffunc int apr_brigade_puts(ap_bucket_brigade *b, const char *str)
+ */
+APU_DECLARE(int) apr_brigade_puts(apr_bucket_brigade *b, const char *str);
+
+/**
+ * This function writes a character into a bucket brigade.
+ * @param b The bucket brigade to add to
+ * @param c The character to add
+ * @return The number of bytes added to the brigade
+ * @deffunc int apr_brigade_putc(apr_bucket_brigade *b, const char c)
+ */
+APU_DECLARE(int) apr_brigade_putc(apr_bucket_brigade *b, const char c);
+
+/**
* This function writes an unspecified number of strings into a bucket brigade.
- * We just allocate a new heap bucket for each string.
* @param b The bucket brigade to add to
* @param ... The strings to add
* @return The number of bytes added to the brigade
@@ -649,7 +680,7 @@
APU_DECLARE_NONSTD(int) apr_brigade_putstrs(apr_bucket_brigade *b, ...);
/**
- * Evaluate a printf and put the resulting string into a bucket at the end
+ * Evaluate a printf and put the resulting string at the end
* of the bucket brigade.
* @param b The brigade to write to
* @param fmt The format of the string to write
@@ -661,7 +692,7 @@
const char *fmt, ...);
/**
- * Evaluate a printf and put the resulting string into a bucket at the end
+ * Evaluate a printf and put the resulting string at the end
* of the bucket brigade.
* @param b The brigade to write to
* @param fmt The format of the string to write
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: brigade patch, last time.
Posted by rb...@covalent.net.
> > > 1) large writes consume large amounts of working set (copy into a bucket)
> > > 2) medium-ish writes cause extra copies (anything larger than 9k cause an
> > > extra copy to the heap, rather than delivery down the chain)
> > > 3) two ordering problems: within the brigade, and between ap_r* and
> > > ap_pass_brigade.
> >
> > The problems you have with 1 are decisions made, not design. So let's
> > ignore those. 2 is also a decision, not a design flaw.
>
> They're important to me since I know they can be solved reasonably (and the
> existing code deals *very* well with them for ap_r*). Any solution needs to
> deal with these issues, whether in the initial patch or a follow-on patch.
I didn't say they weren't important, I said they weren't bugs. And they
aren't, they are decisions that I made. We can change the decision at any
time.
> > > B) The use of r->bb is always going to cause an ordering problem between
> > > ap_r* and ap_pass_brigade.
> > >
> > > Answer: The only solution I see there is to teach ap_pass_brigade about
> > > r->bb. Not sure how I feel about that.
> >
> > Let me give another option. Stop using your own brigade in handlers. If
> > you are re-writing your handler to use brigades, you have two options, use
> > your own brigade, or just use r->bb. If you use your own brigade, then
> > you had better be sure that r->bb is never used, or that you account for
> > it. The core shouldn't have to deal with that.
>
> Hmm. Possible, although I'm not sure that I like it. It still feels a bit
> like a trap door. Specifically: how is this communicated to module
> developers? If we make it so that a module developer can't make an ordering
> mistake no matter how he calls the Apache API, then everybody will be
> happier. I also worry about filter writers beliving they should use r->bb,
> but discovering they're stomping on the brigade that was passed to them.
a) I don't worry about creating API's that don't allow mistakes. I have
never seen such an API. b) If we document that filters can't use r->bb,
but handlers should, that should be enough. Any mistakes made will be
very obvious very quickly.
> Imagine the following:
>
> my_filter(bb):
> b = apr_bucket_create_immortal("please wait while your data is loaded");
> APR_BRIGADE_INSERT_TAIL(flush_bb, b);
> b = apr_bucket_create_flush();
> APR_BRIGADE_INSERT_TAIL(flush_bb, b);
> ap_pass_brigade(flush_bb, b);
>
> b = apr_bucket_create_pipe(...)
> APR_BRIGADE_INSERT_HEAD(bb, b)
> ap_pass_brigade(f->next, bb);
>
> If r->bb is used in there, then things fall apart pretty quick :-) Telling
> people to "handlers should always use r->bb or live at their peril," could
> confuse them ("aren't I handling the filter callback?"). Not to mention that
> most people never read the docs on this stuff :-) A lot of monkey-see,
> monkey-do. Cut/paste from a handler to filter would be troublesome.
>
> etc etc. Not saying I'm hard and fast against it, but it still sounds like
> trouble. I'd like to find a way that we automagically sync between r->bb and
> arbitrary brigade output.
What you are saying, is that developers don't need to understand an API
before using it? I disagree. If I was writing a module that had a
filter, I would personally copy a filter, not a handler. The "aren't I
handling the filter callback" thing is a question of terminology. We have
always called those functions handlers, so any module writer should know
what we mean. BTW, cut/paste from a handler to a filter would be
troublesome now. Imagine copying code with an ap_r* call. You now have
an infinite loop. We can't protect programmers from themselves in all
cases, sometimes we have to accept that our programmers are intelligent
people.
> > If this is acceptable to everybody, I will fix the code tonight, and
> > commit today. This lets us work on fixing any bugs or change decisions
> > over the next week or so. As soon as i see 3 +1's, I will write the code
> > and commit. If I get the 3 +1's today, then you can expect the commit to
> > happen about 10:00 tonight. BTW, I believe this should happen before any
> > kind of beta, because this fixes performance for a filters as well as
> > handlers, and it allows us to simplify the header handling a bit more.
>
> I don't think we need to go in and revamp this stuff for beta. You have been
> pushing for solid code. Changes like this won't be solid at all. Our
> performance is completely fine at this point for our beta. We don't need to
> spiff up filter performance because there aren't any, other than
> mod_include. ... and we don't have perf measurements to determine *if* there
> is a performance problem in mod_include that needs to be solved *before* the
> beta.
This is untrue. I happen to work with a guy (Doug MacEachern) who has a
couple of filters, and he has already implemented this logic
himself. Take a look at mod_perl 2.0. When you are done looking at that,
try looking at mod_snake written by Jon Travis. It has the same
logic. This is what I want to avoid. If we release a beta with this API,
then more module writers will write for this API, and we will force them
to buffer their data in their own code. If we fix it before we go beta,
we stop people from needing to solve this problem.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: brigade patch, last time.
Posted by Greg Stein <gs...@lyra.org>.
On Sat, Jan 27, 2001 at 09:42:40AM -0800, rbb@covalent.net wrote:
>...
> > 1) large writes consume large amounts of working set (copy into a bucket)
> > 2) medium-ish writes cause extra copies (anything larger than 9k cause an
> > extra copy to the heap, rather than delivery down the chain)
> > 3) two ordering problems: within the brigade, and between ap_r* and
> > ap_pass_brigade.
>
> The problems you have with 1 are decisions made, not design. So let's
> ignore those. 2 is also a decision, not a design flaw.
They're important to me since I know they can be solved reasonably (and the
existing code deals *very* well with them for ap_r*). Any solution needs to
deal with these issues, whether in the initial patch or a follow-on patch.
>...
> > B) The use of r->bb is always going to cause an ordering problem between
> > ap_r* and ap_pass_brigade.
> >
> > Answer: The only solution I see there is to teach ap_pass_brigade about
> > r->bb. Not sure how I feel about that.
>
> Let me give another option. Stop using your own brigade in handlers. If
> you are re-writing your handler to use brigades, you have two options, use
> your own brigade, or just use r->bb. If you use your own brigade, then
> you had better be sure that r->bb is never used, or that you account for
> it. The core shouldn't have to deal with that.
Hmm. Possible, although I'm not sure that I like it. It still feels a bit
like a trap door. Specifically: how is this communicated to module
developers? If we make it so that a module developer can't make an ordering
mistake no matter how he calls the Apache API, then everybody will be
happier. I also worry about filter writers beliving they should use r->bb,
but discovering they're stomping on the brigade that was passed to them.
Imagine the following:
my_filter(bb):
b = apr_bucket_create_immortal("please wait while your data is loaded");
APR_BRIGADE_INSERT_TAIL(flush_bb, b);
b = apr_bucket_create_flush();
APR_BRIGADE_INSERT_TAIL(flush_bb, b);
ap_pass_brigade(flush_bb, b);
b = apr_bucket_create_pipe(...)
APR_BRIGADE_INSERT_HEAD(bb, b)
ap_pass_brigade(f->next, bb);
If r->bb is used in there, then things fall apart pretty quick :-) Telling
people to "handlers should always use r->bb or live at their peril," could
confuse them ("aren't I handling the filter callback?"). Not to mention that
most people never read the docs on this stuff :-) A lot of monkey-see,
monkey-do. Cut/paste from a handler to filter would be troublesome.
etc etc. Not saying I'm hard and fast against it, but it still sounds like
trouble. I'd like to find a way that we automagically sync between r->bb and
arbitrary brigade output.
>...
> If this is acceptable to everybody, I will fix the code tonight, and
> commit today. This lets us work on fixing any bugs or change decisions
> over the next week or so. As soon as i see 3 +1's, I will write the code
> and commit. If I get the 3 +1's today, then you can expect the commit to
> happen about 10:00 tonight. BTW, I believe this should happen before any
> kind of beta, because this fixes performance for a filters as well as
> handlers, and it allows us to simplify the header handling a bit more.
I don't think we need to go in and revamp this stuff for beta. You have been
pushing for solid code. Changes like this won't be solid at all. Our
performance is completely fine at this point for our beta. We don't need to
spiff up filter performance because there aren't any, other than
mod_include. ... and we don't have perf measurements to determine *if* there
is a performance problem in mod_include that needs to be solved *before* the
beta.
IMO, let's ship another alpha or beta, and *then* complete this discussion.
Your second sentence above, "lets us work on fixing any bugs" can be done
right now. No need to wait for this stuff. [which I note you and OtherBill
are doing an exemplary job of :-)]
Let's see a release before we worry about (unknown) filter performance.
I'm going to have very spotty access until Wed night while I'm in Seattle
for some business. I would request that we don't have any of these "please
comment within 24 hours or it goes in" types of patches. Especially if they
aren't going to handle the large-write problem; the existing OLD_WRITE
handles that, and I won't be able to assist with rebuilding that capability
on top of new brigade features if it gets tossed.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: brigade patch, last time.
Posted by rb...@covalent.net.
Let's compromise. :-) See below for my idea.
> 1) large writes consume large amounts of working set (copy into a bucket)
> 2) medium-ish writes cause extra copies (anything larger than 9k cause an
> extra copy to the heap, rather than delivery down the chain)
> 3) two ordering problems: within the brigade, and between ap_r* and
> ap_pass_brigade.
The problems you have with 1 are decisions made, not design. So let's
ignore those. 2 is also a decision, not a design flaw. 3, we disagree
about. Since the only thing that really matters is 3, lets focus on
that. BTW, when I say decision, that basically means that there were two
options. I chose one. I had a reason, whether good or bad, it was a
decision made. Once this is committed, we can re-visit those decisions
and come up with a consensus.
cases B & C are the two that deal with issue #3 above, so lets focus
there.
> B) The use of r->bb is always going to cause an ordering problem between
> ap_r* and ap_pass_brigade.
>
> Answer: The only solution I see there is to teach ap_pass_brigade about
> r->bb. Not sure how I feel about that.
Let me give another option. Stop using your own brigade in handlers. If
you are re-writing your handler to use brigades, you have two options, use
your own brigade, or just use r->bb. If you use your own brigade, then
you had better be sure that r->bb is never used, or that you account for
it. The core shouldn't have to deal with that.
> C) Lastly, the ordering problem within the brigade will continue as long as
> the buffer is stored outside of the bucket list.
>
> Answer: using a buffer bucket (or an overallocated heap bucket) solves
> this one.
I disagree about this. We are dealing with buffering here, and going from
buffered to non-buffered ususally requires that the programmer get
involved. I also have a problem with the definitions, buckets have been
defined to be read-only, we are about to create a bucket that is
read/write. I worry about confusion at that point. I also worry about
how complex the code will need to be, since I have written this code once
before.
> So... if we can agree on (C), then I see the light at end of the tunnel. The
> rest is all doable.
So, here is the compromise. I will re-write the code tonight to use a
bucket for the buffering. This removes the mode. I would ask you to
accept that we can help programmers who use their own brigade and ap_r*
functions. Those people need to be responsible for their handlers, and we
will document that handlers can, and in fact _should_, use r->bb as their
brigade.
If this is acceptable to everybody, I will fix the code tonight, and
commit today. This lets us work on fixing any bugs or change decisions
over the next week or so. As soon as i see 3 +1's, I will write the code
and commit. If I get the 3 +1's today, then you can expect the commit to
happen about 10:00 tonight. BTW, I believe this should happen before any
kind of beta, because this fixes performance for a filters as well as
handlers, and it allows us to simplify the header handling a bit more.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: brigade patch, last time.
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Jan 26, 2001 at 03:00:59PM -0800, rbb@covalent.net wrote:
>
> This patch solves multiple problems.
>
> 1) buffering brigade output.
> 2) ap_r* uses the buffered brigade functions.
> 3) implements a group of ap_f* functions which are the general case of
> ap_r*
> 4) Solves the http_header filter problem, by using the general brigade
> functions.
>
> The only reason for not using this patch that I can see, is that it is
> possible to get data out of order. However, with some diligence, that is
> easy to avoid. The only way this will be an issue, is if somebody decides
> they want to use the bucket functions directly. If they are doing that,
> then they will need to use the apr_brigade_flush function first.
>
> Can people comment on this, so that I can commit it?
While this is neat, I believe it still has problems:
1) large writes consume large amounts of working set (copy into a bucket)
2) medium-ish writes cause extra copies (anything larger than 9k cause an
extra copy to the heap, rather than delivery down the chain)
3) two ordering problems: within the brigade, and between ap_r* and
ap_pass_brigade.
Adding buffering to the brigades is a very laudable goal, and I'd like to
see you solve it. Briefly, there are three improvements that I see:
A) Without the mechanism for the brigade-buffering to flush to the network,
you'll always fall into problems (1) and (2).
Answer: This was the part I mentioned about passing a flush function and
ctx. The brigade functions could then cause delivery of excessive
data to be pushed down the chain. Note that it would also be
possible for a user to pass NULL, meaning "screw it. just copy
the damned thing." Apache itself just plays smart :-)
B) The use of r->bb is always going to cause an ordering problem between
ap_r* and ap_pass_brigade.
Answer: The only solution I see there is to teach ap_pass_brigade about
r->bb. Not sure how I feel about that.
C) Lastly, the ordering problem within the brigade will continue as long as
the buffer is stored outside of the bucket list.
Answer: using a buffer bucket (or an overallocated heap bucket) solves
this one.
I worry about ordering problems because they impose a "mode" within the
system. If you use one mode, then you have to continue using it. If you want
to switch (if you are even aware that you're switching!), then particular
care is needed to ensure proper operation. These subtle requirements are the
things that bite us.
[ example: putting ap_basic_http_header after ap_set_keepalive had a subtle
error w.r.t. force-response-1.0 (and proxies?)
example: moving ap_set_keepalive after the r->chunked test
example: storing "multipart" into r->content_type, thereby blowing away
the ability to put the content-type into the parts.
Each of the above examples were caused by subtle, non-obvious interactions
between different parts of the code. I fear the same for output ordering.
]
In truth, the one that worries me the most is (B). I know that (A) can be
solved in a pretty straight-forward fashion (just two more params to each
brigade buffering function). Item (C) is a design issue of the brigade
interface (the solution is technically easy; agreeing on it is a (ahem)
"controversial" design question).
That leaves an answer for (B). We can't toss the brigade down the chain
after each ap_r* call (that means no buffering). The only answer that I see
is to "hack" into ap_pass_brigade to make it away of r->bb's existence. At
the moment, I'm uncomfortable with that. There may be a different way to
look at the semantics of ap_pass_brigade and r->bb that makes it more
palatable.
Oh. Here we go:
ap_req_output(r, brigade)
This function is used by handlers (filters continue with ap_pass_brigade).
The function is simple:
{
if (r->bb)
prepend(r->bb, brigade) /* r->bb prepended, then emptied */
ap_pass_brigade(r->output_filters, brigade)
}
That might work for problem (B). It falls down if people accidentally use
ap_pass_brigade() [like they do now].
[ but fairly speaking: OLD_WRITE is disabled if people don't play by the
filter rules; everything has the pathological bad case ]
So... if we can agree on (C), then I see the light at end of the tunnel. The
rest is all doable.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/