patch attached
On Tue, Nov 6, 2012 at 10:45 PM, Lieven Govaerts <l...@mobsol.be> wrote:
> Hi,
>
> On Tue, Nov 6, 2012 at 6:13 PM, Lieven Govaerts <l...@mobsol.be> wrote:
>> Hi,
>>
>> On Tue, Nov 6, 2012 at 4:50 PM, Lieven Govaerts <l...@mobsol.be> wrote:
>>> Ben,
>>>
>>> On Tue, Nov 6, 2012 at 4:09 PM, Ben Reser <b...@reser.org> wrote:
>>>> I worked with Philip today and was able to reproduce the exact problem
>>>> he's been seeing. I ended up having to get his full httpd.conf to
>>>> figure it out..
>>>>
>>>> Ultimately the problem proved to be that he had this directive:
>>>> Timeout 3
>>>>
>>>> Which would mean if we don't tend a connection for 3 seconds Apache
>>>> will close it. Serf should be able to deal with the connection being
>>>> closed.
>>>>
>>
>> okay, so with the Timeout directive added I can reproduce this issue.
>>
>> What I see is that the server closes the connection in the middle of
>> sending a response to the client. It doesn't even finalize the
>> response first.
>> So ra_serf is reading the data from the response bucket, but gets an
>> APR_EOF when it needs more data than specified in the Content-Length
>> header of the response.
>>
>> What is the expected behavior here, let serf close down the connection
>> and try the request again on a new connection?
>
> IMHO, the reasonable thing to do here, is to report the error
> correctly to the user, so that we don't corrupt the working copy.
> This way the user can retry the transaction.
>
> For response bodies that are encoded (chunked or gzip/deflated), serf
> will already return an error status to svn/ra_serf. The one case where
> serf silently returns APR_EOF is when no encoding is used so the
> Content-Length header is set.
> Attached patch to serf trunk will calculate the actually read bytes
> from the response body, and ensures an error is returned to svn when
> this is less than what's expected. The patch is not final, I probably
> want to use another error code.
>
> Philip or Ben, can you test that with this patch svn will always stop
> with a communication error?
>
> Lieven
Index: buckets/response_buckets.c
===================================================================
--- buckets/response_buckets.c (revision 1674)
+++ buckets/response_buckets.c (working copy)
@@ -41,6 +41,12 @@ typedef struct {
int chunked; /* Do we need to read trailers? */
int head_req; /* Was this a HEAD request? */
+
+ int check_length; /* Indicates we need calculate # of bytes in
the
+ body and return error if it's too short. */
+ apr_size_t exp_body_len; /* expected size of body, 0 if not known. */
+ apr_size_t actual_body_len; /* Keep track of actual bytes read from the
body
+ to see if we received the full response. */
} response_context_t;
@@ -57,6 +63,9 @@ serf_bucket_t *serf_bucket_response_create(
ctx->state = STATE_STATUS_LINE;
ctx->chunked = 0;
ctx->head_req = 0;
+ ctx->check_length = 0;
+ ctx->exp_body_len = 0;
+ ctx->actual_body_len = 0;
serf_linebuf_init(&ctx->linebuf);
@@ -251,6 +260,8 @@ static apr_status_t run_machine(serf_bucket_t *bkt
}
ctx->body = serf_bucket_limit_create(ctx->body, length,
bkt->allocator);
+ ctx->check_length = 1;
+ ctx->exp_body_len = length;
}
else {
v = serf_bucket_headers_get(ctx->headers, "Transfer-Encoding");
@@ -268,6 +279,8 @@ static apr_status_t run_machine(serf_bucket_t *bkt
}
v = serf_bucket_headers_get(ctx->headers, "Content-Encoding");
if (v) {
+ ctx->check_length = 0; /* deflate bucket will check this. */
+
/* Need to handle multiple content-encoding. */
if (v && strcasecmp("gzip", v) == 0) {
ctx->body =
@@ -385,6 +398,12 @@ static apr_status_t serf_response_read(serf_bucket
}
rv = serf_bucket_read(ctx->body, requested, data, len);
+ if (SERF_BUCKET_READ_ERROR(rv))
+ return rv;
+
+ if (ctx->check_length)
+ ctx->actual_body_len += *len;
+
if (APR_STATUS_IS_EOF(rv)) {
if (ctx->chunked) {
ctx->state = STATE_TRAILERS;
@@ -392,6 +411,8 @@ static apr_status_t serf_response_read(serf_bucket
rv = APR_SUCCESS;
}
else {
+ if (ctx->check_length && ctx->actual_body_len < ctx->exp_body_len)
+ return SERF_ERROR_BAD_HTTP_RESPONSE;
ctx->state = STATE_DONE;
}
}
