You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Justin Erenkrantz <ju...@erenkrantz.com> on 2011/05/16 12:00:31 UTC
[PATCH] Have serf send Content-Length request bodies
So...at my hotel in Ljubljana, they have a silly Squid proxy that
doesn't understand chunked request bodies. I know this has been a
huge detriment for some people with serf - so, here's a patch for serf
that will try to send C-L bodies for basic HTTP request bodies. With
this patch, ra_serf only sends C-L...however, the hotel's version of
Squid (squid/2.7.STABLE9) doesn't understand chunked *response* bodies
either. So, it ends up "Connection: close"-ing after *every single
request*. Needless to say, serf's not amused - it still works just
fine; but it's pretty slow as it has to constantly re-open TCP
connections. There's nothing serf can do about directly controlling
the server-side's logic to avoid chunking responses.
So, I'm on the fence about cleaning up and applying this patch as I
think if you're talking to a dumb HTTP proxy, you're likely going to
run into other serious issues.
Thoughts? -- justin
Index: serf_bucket_util.h
===================================================================
--- serf_bucket_util.h (revision 1462)
+++ serf_bucket_util.h (working copy)
@@ -79,6 +79,10 @@ serf_bucket_t *serf_default_read_bucket(
serf_bucket_t *bucket,
const serf_bucket_type_t *type);
+apr_status_t serf_default_advise_length(
+ serf_bucket_t *bucket,
+ apr_size_t *len);
+
/**
* Default implementation of the @see destroy functionality.
*
Index: serf.h
===================================================================
--- serf.h (revision 1465)
+++ serf.h (working copy)
@@ -814,6 +814,10 @@ struct serf_bucket_type_t {
const char **data, apr_size_t *len);
/**
+ */
+ apr_status_t (*advise_length)(serf_bucket_t *bucket, apr_size_t *len);
+
+ /**
* Destroy @a bucket, along with any associated resources.
*/
void (*destroy)(serf_bucket_t *bucket);
@@ -882,6 +886,7 @@ struct serf_bucket_type_t {
SERF__RECREAD(b, (b)->type->read_for_sendfile(b,r,h,f,o,l))
#define serf_bucket_read_bucket(b,t) ((b)->type->read_bucket(b,t))
#define serf_bucket_peek(b,d,l) ((b)->type->peek(b,d,l))
+#define serf_bucket_advise_length(b,l) ((b)->type->advise_length(b,l))
#define serf_bucket_destroy(b) ((b)->type->destroy(b))
#define serf_bucket_snapshot(b) ((b)->type->snapshot(b))
#define serf_bucket_restore_snapshot(b) ((b)->type->restore_snapshot(b))
Index: buckets/limit_buckets.c
===================================================================
--- buckets/limit_buckets.c (revision 1462)
+++ buckets/limit_buckets.c (working copy)
@@ -118,6 +118,7 @@ const serf_bucket_type_t serf_bucket_type_limit =
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_limit_peek,
+ serf_default_advise_length,
serf_limit_destroy,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/file_buckets.c
===================================================================
--- buckets/file_buckets.c (revision 1462)
+++ buckets/file_buckets.c (working copy)
@@ -105,6 +105,22 @@ static apr_status_t serf_file_peek(serf_bucket_t *
return serf_databuf_peek(&ctx->databuf, data, len);
}
+static apr_status_t serf_file_advise_length(serf_bucket_t *bucket,
+ apr_size_t *len)
+{
+ file_context_t *ctx = bucket->data;
+ apr_finfo_t finfo;
+ apr_status_t status;
+
+ status = apr_file_info_get(&finfo, APR_FINFO_SIZE, ctx->file);
+
+ if (!status) {
+ *len = finfo.size;
+ }
+
+ return APR_SUCCESS;
+}
+
const serf_bucket_type_t serf_bucket_type_file = {
"FILE",
serf_file_read,
@@ -113,6 +129,7 @@ const serf_bucket_type_t serf_bucket_type_file = {
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_file_peek,
+ serf_file_advise_length,
serf_default_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/buckets.c
===================================================================
--- buckets/buckets.c (revision 1462)
+++ buckets/buckets.c (working copy)
@@ -104,6 +104,12 @@ serf_bucket_t *serf_default_read_bucket(
return NULL;
}
+apr_status_t serf_default_advise_length(
+ serf_bucket_t *bucket,
+ apr_size_t *len)
+{
+ return APR_ENOTIMPL;
+}
void serf_default_destroy(serf_bucket_t *bucket)
{
Index: buckets/dechunk_buckets.c
===================================================================
--- buckets/dechunk_buckets.c (revision 1462)
+++ buckets/dechunk_buckets.c (working copy)
@@ -181,6 +181,7 @@ const serf_bucket_type_t serf_bucket_type_dechunk
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_dechunk_peek,
+ serf_default_advise_length,
serf_dechunk_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/aggregate_buckets.c
===================================================================
--- buckets/aggregate_buckets.c (revision 1465)
+++ buckets/aggregate_buckets.c (working copy)
@@ -370,6 +370,28 @@ static apr_status_t serf_aggregate_peek(serf_bucke
return APR_ENOTIMPL;
}
+static apr_status_t serf_aggregate_advise_length(serf_bucket_t *bucket,
+ apr_size_t *len)
+{
+ aggregate_context_t *ctx = bucket->data;
+ bucket_list_t *l = ctx->list;
+
+ *len = 0;
+ while (l) {
+ apr_status_t status;
+ apr_size_t bucket_len;
+
+ status = serf_bucket_advise_length(l->bucket, &bucket_len);
+ if (status) {
+ return status;
+ }
+ *len += bucket_len;
+ l = l->next;
+ }
+
+ return APR_SUCCESS;
+}
+
static serf_bucket_t * serf_aggregate_read_bucket(
serf_bucket_t *bucket,
const serf_bucket_type_t *type)
@@ -462,6 +484,7 @@ const serf_bucket_type_t serf_bucket_type_aggregat
serf_default_read_for_sendfile,
serf_aggregate_read_bucket,
serf_aggregate_peek,
+ serf_aggregate_advise_length,
serf_aggregate_destroy_and_data,
serf_aggregate_snapshot,
serf_aggregate_restore_snapshot,
Index: buckets/barrier_buckets.c
===================================================================
--- buckets/barrier_buckets.c (revision 1462)
+++ buckets/barrier_buckets.c (working copy)
@@ -93,6 +93,7 @@ const serf_bucket_type_t serf_bucket_type_barrier
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_barrier_peek,
+ serf_default_advise_length,
serf_barrier_destroy,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/request_buckets.c
===================================================================
--- buckets/request_buckets.c (revision 1462)
+++ buckets/request_buckets.c (working copy)
@@ -112,9 +112,23 @@ static void serialize_data(serf_bucket_t *bucket)
serf_bucket_aggregate_append(bucket, new_bucket);
serf_bucket_aggregate_append(bucket, ctx->headers);
if (ctx->body != NULL) {
- /* Morph the body bucket to a chunked encoding bucket for now. */
- serf_bucket_headers_set(ctx->headers, "Transfer-Encoding", "chunked");
- ctx->body = serf_bucket_chunk_create(ctx->body, bucket->allocator);
+ apr_status_t body_len_status;
+ apr_size_t body_len;
+
+ body_len_status = serf_bucket_advise_length(ctx->body, &body_len);
+ if (!body_len_status) {
+ char len_hdr[20];
+ apr_size_t len_hdr_len;
+
+ len_hdr_len = apr_snprintf(len_hdr, sizeof(len_hdr),
+ "%" APR_SIZE_T_FMT, body_len);
+
+ serf_bucket_headers_set(ctx->headers, "Content-Length", len_hdr);
+ } else {
+ /* Morph the body bucket to a chunked encoding bucket for now. */
+ serf_bucket_headers_set(ctx->headers,
"Transfer-Encoding", "chunked");
+ ctx->body = serf_bucket_chunk_create(ctx->body, bucket->allocator);
+ }
serf_bucket_aggregate_append(bucket, ctx->body);
}
@@ -199,6 +213,7 @@ const serf_bucket_type_t serf_bucket_type_request
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_request_peek,
+ serf_default_advise_length,
serf_default_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/socket_buckets.c
===================================================================
--- buckets/socket_buckets.c (revision 1462)
+++ buckets/socket_buckets.c (working copy)
@@ -110,6 +110,7 @@ const serf_bucket_type_t serf_bucket_type_socket =
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_socket_peek,
+ serf_default_advise_length,
serf_default_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/simple_buckets.c
===================================================================
--- buckets/simple_buckets.c (revision 1462)
+++ buckets/simple_buckets.c (working copy)
@@ -122,6 +122,16 @@ static apr_status_t serf_simple_peek(serf_bucket_t
return APR_EOF;
}
+static apr_status_t serf_simple_advise_length(serf_bucket_t *bucket,
+ apr_size_t *len)
+{
+ simple_context_t *ctx = bucket->data;
+
+ *len = ctx->remaining;
+
+ return APR_SUCCESS;
+}
+
static void serf_simple_destroy(serf_bucket_t *bucket)
{
simple_context_t *ctx = bucket->data;
@@ -169,6 +179,7 @@ const serf_bucket_type_t serf_bucket_type_simple =
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_simple_peek,
+ serf_simple_advise_length,
serf_simple_destroy,
serf_simple_snapshot,
serf_simple_restore_snapshot,
Index: buckets/mmap_buckets.c
===================================================================
--- buckets/mmap_buckets.c (revision 1462)
+++ buckets/mmap_buckets.c (working copy)
@@ -106,6 +106,16 @@ static apr_status_t serf_mmap_peek(serf_bucket_t *
return APR_ENOTIMPL;
}
+static apr_status_t serf_mmap_advise_length(serf_bucket_t *bucket,
+ apr_size_t *len)
+{
+ mmap_context_t *ctx = bucket->data;
+
+ *len = ctx->remaining;
+
+ return APR_SUCCESS;
+}
+
const serf_bucket_type_t serf_bucket_type_mmap = {
"MMAP",
serf_mmap_read,
@@ -114,6 +124,7 @@ const serf_bucket_type_t serf_bucket_type_mmap = {
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_mmap_peek,
+ serf_mmap_advise_length,
serf_default_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/response_buckets.c
===================================================================
--- buckets/response_buckets.c (revision 1462)
+++ buckets/response_buckets.c (working copy)
@@ -425,6 +425,7 @@ const serf_bucket_type_t serf_bucket_type_response
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_response_peek,
+ serf_default_advise_length,
serf_response_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/ssl_buckets.c
===================================================================
--- buckets/ssl_buckets.c (revision 1467)
+++ buckets/ssl_buckets.c (working copy)
@@ -1488,6 +1488,7 @@ const serf_bucket_type_t serf_bucket_type_ssl_encr
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_ssl_peek,
+ serf_default_advise_length,
serf_ssl_encrypt_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
@@ -1502,6 +1503,7 @@ const serf_bucket_type_t serf_bucket_type_ssl_decr
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_ssl_peek,
+ serf_default_advise_length,
serf_ssl_decrypt_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/deflate_buckets.c
===================================================================
--- buckets/deflate_buckets.c (revision 1462)
+++ buckets/deflate_buckets.c (working copy)
@@ -372,6 +372,7 @@ const serf_bucket_type_t serf_bucket_type_deflate
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_deflate_peek,
+ serf_default_advise_length,
serf_deflate_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/iovec_buckets.c
===================================================================
--- buckets/iovec_buckets.c (revision 1462)
+++ buckets/iovec_buckets.c (working copy)
@@ -186,6 +186,7 @@ const serf_bucket_type_t serf_bucket_type_iovec =
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_iovec_peek,
+ serf_default_advise_length,
serf_iovec_destroy,
serf_iovec_snapshot,
serf_iovec_restore_snapshot,
Index: buckets/chunk_buckets.c
===================================================================
--- buckets/chunk_buckets.c (revision 1462)
+++ buckets/chunk_buckets.c (working copy)
@@ -231,6 +231,7 @@ const serf_bucket_type_t serf_bucket_type_chunk =
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_chunk_peek,
+ serf_default_advise_length,
serf_chunk_destroy,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/headers_buckets.c
===================================================================
--- buckets/headers_buckets.c (revision 1462)
+++ buckets/headers_buckets.c (working copy)
@@ -425,6 +425,7 @@ const serf_bucket_type_t serf_bucket_type_headers
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_headers_peek,
+ serf_default_advise_length,
serf_headers_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Index: buckets/bwtp_buckets.c
===================================================================
--- buckets/bwtp_buckets.c (revision 1462)
+++ buckets/bwtp_buckets.c (working copy)
@@ -306,6 +306,7 @@ const serf_bucket_type_t serf_bucket_type_bwtp_fra
serf_default_read_for_sendfile,
serf_default_read_bucket,
serf_bwtp_frame_peek,
+ serf_default_advise_length,
serf_default_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
@@ -595,6 +596,7 @@ const serf_bucket_type_t serf_bucket_type_bwtp_inc
serf_default_read_for_sendfile,
serf_default_read_bucket,
bwtp_incoming_peek,
+ serf_default_advise_length,
bwtp_incoming_destroy_and_data,
serf_default_snapshot,
serf_default_restore_snapshot,
Re: [PATCH] Have serf send Content-Length request bodies
Posted by Konstantin Kolinko <kn...@gmail.com>.
2011/5/17 Justin Erenkrantz <ju...@erenkrantz.com>:
> On Mon, May 16, 2011 at 10:48 PM, Konstantin Kolinko
> <kn...@gmail.com> wrote:
>> HTTP/1.0 does not support keep-alive, and thus the connection will be
>> closed after each request. You will need HTTP/1.1 to keep the
>> connection open.
>
> Correct - in this particular set of circumstances, httpd is going to
> either select Connection: Close or chunked response encoding. Even if
> serf were to advertise that it were only 1.0, httpd would then still
> choose Connection: close (as it doesn't know the response lengths).
> So, since Squid doesn't know what chunked is...httpd is forced to
> close the connection.
>
>> Maybe you are able to connect with HTTPS?
>
> Of course - SSL works just fine...even better with my last set of
> patches over the weekend. =)
>
>> Quick look at the docs says that HTTP/1.1 is disabled in Squid 2.7 by
>> default and its implementation "is still incomplete" [1]. Squid 3.x
>> should behave better.
>>
>> [1] http://www.squid-cache.org/Doc/config/server_http11/
>>
>> I am not a Squid admin, but just verifying my own worries about that product.
>
> Yes, I'm aware that squid does not adhere to any type of RFC compliance.
>
> The fundamental question is: is it better to have it work and perform
> terribly, or have it fail and highlight to the user that they would be
> better off using HTTPS or something else.
>
> I can support both sides of the argument...hence, my desire to get
> feedback. -- justin
My personal opinion (from position of a svn user) is that HTTP/1.1
support is necessary for proper operation.
Rationale behind supporting HTTP/1.0 in the library might be
1) improvement of the serf library per se
2) some simple commands might work, much like when svn repository is
accessed with a web browser. svn list? svn cat?
Best regards,
Konstantin Kolinko
(I am not subscribed to serf-dev@. It will bounce off.)
Re: [PATCH] Have serf send Content-Length request bodies
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Mon, May 16, 2011 at 10:48 PM, Konstantin Kolinko
<kn...@gmail.com> wrote:
> HTTP/1.0 does not support keep-alive, and thus the connection will be
> closed after each request. You will need HTTP/1.1 to keep the
> connection open.
Correct - in this particular set of circumstances, httpd is going to
either select Connection: Close or chunked response encoding. Even if
serf were to advertise that it were only 1.0, httpd would then still
choose Connection: close (as it doesn't know the response lengths).
So, since Squid doesn't know what chunked is...httpd is forced to
close the connection.
> Maybe you are able to connect with HTTPS?
Of course - SSL works just fine...even better with my last set of
patches over the weekend. =)
> Quick look at the docs says that HTTP/1.1 is disabled in Squid 2.7 by
> default and its implementation "is still incomplete" [1]. Squid 3.x
> should behave better.
>
> [1] http://www.squid-cache.org/Doc/config/server_http11/
>
> I am not a Squid admin, but just verifying my own worries about that product.
Yes, I'm aware that squid does not adhere to any type of RFC compliance.
The fundamental question is: is it better to have it work and perform
terribly, or have it fail and highlight to the user that they would be
better off using HTTPS or something else.
I can support both sides of the argument...hence, my desire to get
feedback. -- justin
Re: [PATCH] Have serf send Content-Length request bodies
Posted by Konstantin Kolinko <kn...@gmail.com>.
2011/5/16 Justin Erenkrantz <ju...@erenkrantz.com>:
> So...at my hotel in Ljubljana, they have a silly Squid proxy that
> doesn't understand chunked request bodies. I know this has been a
> huge detriment for some people with serf - so, here's a patch for serf
> that will try to send C-L bodies for basic HTTP request bodies. With
> this patch, ra_serf only sends C-L...however, the hotel's version of
> Squid (squid/2.7.STABLE9) doesn't understand chunked *response* bodies
> either. So, it ends up "Connection: close"-ing after *every single
> request*. Needless to say, serf's not amused - it still works just
> fine; but it's pretty slow as it has to constantly re-open TCP
> connections. There's nothing serf can do about directly controlling
> the server-side's logic to avoid chunking responses.
HTTP/1.0 does not support keep-alive, and thus the connection will be
closed after each request. You will need HTTP/1.1 to keep the
connection open.
Maybe you are able to connect with HTTPS?
Quick look at the docs says that HTTP/1.1 is disabled in Squid 2.7 by
default and its implementation "is still incomplete" [1]. Squid 3.x
should behave better.
[1] http://www.squid-cache.org/Doc/config/server_http11/
I am not a Squid admin, but just verifying my own worries about that product.
>
> So, I'm on the fence about cleaning up and applying this patch as I
> think if you're talking to a dumb HTTP proxy, you're likely going to
> run into other serious issues.
>
> Thoughts? -- justin
>
Best regards,
Konstantin Kolinko
Sending from dev@subversion. I see that serf-dev@ is also on CC list.