Here's the new patch incorporating the feedback.
Comments inline.
One thing that I'd like feedback : The eoc bucket is inserted by the ap_end_connection() function(in server/connection.c) - and deleted by SSL in ssl_engine_io.c. I don't feel comfortable with it.
Is that okay ? If not, what is a good place for inserting the EOC bucket (in mod_ssl) ?
Agreed. It needs to be passed on to the next filter as well.
Index: server/connection.c =================================================================== RCS file: /home/cvspublic/httpd-2.0/server/connection.c,v retrieving revision 1.114 diff -u -r1.114 connection.c --- server/connection.c 9 Feb 2004 20:40:49 -0000 1.114 +++ server/connection.c 25 Feb 2004 00:41:18 -0000 @@ -65,10 +65,23 @@ #define MAX_SECS_TO_LINGER 30 #endif
+AP_CORE_DECLARE(void) ap_end_connection(conn_rec *c) +{ + apr_bucket_brigade *bb; + apr_bucket *b; + + bb = apr_brigade_create(c->pool, c->bucket_alloc); + b = ap_bucket_eoc_create(c->bucket_alloc); + APR_BRIGADE_INSERT_TAIL(bb, b); + ap_pass_brigade(c->output_filters, bb); +} + AP_CORE_DECLARE(void) ap_flush_conn(conn_rec *c) { apr_bucket_brigade *bb; apr_bucket *b; + + ap_end_connection(c);
I wouldn't split this out into two functions that both create a brigade and pass it. I'd just create the EOC bucket and stick it *after* the flush bucket (see below for why I think that ordering makes sense).
bb = apr_brigade_create(c->pool, c->bucket_alloc);
b = apr_bucket_flush_create(c->bucket_alloc);
Index: modules/ssl/ssl_engine_io.c =================================================================== RCS file: /home/cvspublic/httpd-2.0/modules/ssl/ssl_engine_io.c,v retrieving revision 1.117 diff -u -r1.117 ssl_engine_io.c --- modules/ssl/ssl_engine_io.c 9 Feb 2004 20:29:22 -0000 1.117 +++ modules/ssl/ssl_engine_io.c 25 Feb 2004 01:16:28 -0000 @@ -100,6 +100,7 @@ BIO *pbioWrite; ap_filter_t *pInputFilter; ap_filter_t *pOutputFilter; + int nobuffer; /* non-zero to prevent buffering */ } ssl_filter_ctx_t;
typedef struct { @@ -193,7 +194,8 @@ */ BIO_clear_retry_flags(bio);
- if (!outctx->length && (inl + outctx->blen < sizeof(outctx->buffer))) { + if (!outctx->length && (inl + outctx->blen < sizeof(outctx->buffer)) && + !outctx->filter_ctx->nobuffer) { /* the first two SSL_writes (of 1024 and 261 bytes) * need to be in the same packet (vec[0].iov_base) */ @@ -1394,6 +1396,14 @@ */ apr_bucket_delete(bucket); } + } + else if (AP_BUCKET_IS_EOC(bucket)) { + /* The special "EOC" bucket means a shutdown is needed; + * turn off buffering in bio_filter_out_write. + */ + filter_ctx->nobuffer = 1; + status = ssl_filter_io_shutdown(filter_ctx, f->c, 0); + apr_bucket_delete(bucket);
Actually, I don't think we need nobuffer. Or, could we end up being recursively called when we call shutdown? (I forget the call ordering here.) Our contract can state that there will be no more data after an EOC. This is much the same as there should be no data after an EOS to a request-level filter.
I would also not delete this bucket, I'd just try to leave it there to pass on to the next filter. I'd want the core_conn_filter to see the EOC as well. That *might* mean the ordering should be FLUSH *then* EOC instead of EOC *then* FLUSH (as you have it now).
Thanks! -- justin
