Serf doesn't handle an incoming 408 Request Timeout

[GoogleCodeExporter]

The issue was first reported in Subversion:
http://svn.haxx.se/dev/archive-2012-11/0076.shtml

* It is possible to reproduce the problem:
1. Use mod_reqtimeout in apache (probably enabled by default) 
2. Add this config to the apache config file:
RequestReadTimeout header=1 body=2 
This will give the client 1 second to send the headers of the request, and 2 
for the body.
3. Apply this patch to delay writing the request message:
Index: outgoing.c
===================================================================
--- outgoing.c  (revision 1678)
+++ outgoing.c  (working copy)
@@ -22,6 +22,8 @@

 #include "serf_private.h"

+#include <unistd.h> /* sleep */
+
 /* cleanup for sockets */
 static apr_status_t clean_skt(void *data)
 {
@@ -493,8 +495,10 @@
     apr_size_t written;
     apr_status_t status;

+    /* DEBUG: fake timeout after header/body. */
     status = apr_socket_sendv(conn->skt, conn->vec,
-                              conn->vec_len, &written);
+                              1, &written);
+    sleep(5);
        if (status && !APR_STATUS_IS_EAGAIN(status))
         serf__log_skt(SOCK_VERBOSE, __FILE__, conn->skt,
                       "socket_sendv error %d\n", status);

* What is the expected output? What do you see instead?
When you know send a request to the server, serf will take so long to send the 
request that a timeout triggers in apache. Apache will then send a 408 Request 
Timeout response.

For serf this response is unexpected, as it hasn't finished writing the request 
yet. It will return an APR_EGENERAL error from serf_context_run. This is an 
error that the application can't handle, in case of svn it will stop and report 
the error to the user.

* Proposed solution:
- Serf can identify the unexpected response as a 408 response
- It should pass it to the application via the normal response handler. 
Currently this handler is only called when the request has been sent 
completely, so need to check if the API allows it.
- The application can then decide to bail out, retry the request once ...
   Note: I first thought to let serf handle this internally, but we might get in an endless loop retrying a request without the application being aware what's going on.

Original issue reported on code.google.com by lieven.govaerts@gmail.com on 8 Nov 2012 at 7:57

[GoogleCodeExporter]

Extra info:
With the proposed solution serf has to read the response status line first and 
analyzing it, before passing it to the application. This is similar to our 
authentication handlers, that have to read status line and headers first to 
check if the response can be handled internally in serf.

The response bucket needed for this is provided by the application in the 
serf_response_acceptor_t callback. In practice the application will probably 
create a serf response_bucket, but that is not guaranteed.
So we'll need a mechanism that allows serf to wrap the incoming stream with a 
response_bucket in order to read status line & headers. If then turns out that 
the response needs to be passed to the application, the data already read in 
the response_bucket needs to be made available again on the stream passed to 
the application.

Original comment by lieven.govaerts@gmail.com on 8 Nov 2012 at 8:04

[GoogleCodeExporter]

I do not understand why we need read status line? I think we should fix serf to 
handle reading response before sending all request and then pass it to 
application as a normal error response.

Original comment by chemodax@gmail.com on 8 Nov 2012 at 8:59

[GoogleCodeExporter]

You're assuming that it's acceptable for a server to start responding to a 
request before that request has been fully received. This is not what rfc 2616 
says, and serf has been quite strict in implementing this as a rule.

I'm not against what you're suggesting, if that is at least in practice 
acceptable behavior.

Original comment by lieven.govaerts@gmail.com on 8 Nov 2012 at 9:12

[GoogleCodeExporter]

One extra observation from looking at the code: serf currently drops unexpected 
incoming data on a connection, if that data was received in the same bucket 
read as the APR_EOF result was returned.

See outgoing.c/read_from_connection line 962:
            status = serf_bucket_read(conn->stream, SERF_READ_ALL_AVAIL,
                                      &data, &len);
            if (!status && len) {
                status = APR_EGENERAL;
            }
            else if (APR_STATUS_IS_EOF(status)) {
                reset_connection(conn, 1);
                status = APR_SUCCESS;
            }

Original comment by lieven.govaerts@gmail.com on 8 Nov 2012 at 9:16

[GoogleCodeExporter]

I didn't find where rfc 2616 specifies that response cannot be transmitted 
before receiving all request. 

Original comment by chemodax@gmail.com on 8 Nov 2012 at 10:54

[GoogleCodeExporter]

[[[
8.2.2 Monitoring Connections for Error Status Messages

An HTTP/1.1 (or later) client sending a message-body SHOULD monitor the network 
connection for an error status while it is transmitting the request. If the 
client sees an error status, it SHOULD immediately cease transmitting the body. 
If the body is being sent using a "chunked" encoding (section 3.6), a zero 
length chunk and empty trailer MAY be used to prematurely mark the end of the 
message. If the body was preceded by a Content-Length header, the client MUST 
close the connection. 
]]]

Original comment by chemodax@gmail.com on 8 Nov 2012 at 11:04

[GoogleCodeExporter]

Ivan, interesting section.

However, I don't see it making a general case for allowing response to come in 
while the request is in progress.

It specifically mentions "Error Status Messages", this mechanism is only 
allowed for 4xx or 5xx status codes, but not for 2xx etc. Suppose a server 
starts responding to a request with a 200 result, what would we do with the 
request when the response has been read completely? Stop sending it? Keep 
sending it?
At least in error status it's clear that we should abort writing the request.

So I still think we should parse the status line, but based on RFC 2616 8.2.2 
we should broaden the scope of this issue to all 4xx and 5xx responses, not 
only 408. 

Original comment by lieven.govaerts@gmail.com on 9 Nov 2012 at 7:44

[GoogleCodeExporter]

"I do not understand why we need read status line? I think we should fix serf 
to handle reading response before sending all request and then pass it to 
application as a normal error response."

I have been thinking about this in traffic this morning, and came to the 
conclusion that I can agree with your suggestion. Even though the RFC only 
allows it for status messages, if a server is implemented differently and start 
responding before a request is fully transmitted it's not serf's job to hide 
that content from the application.

Original comment by lieven.govaerts@gmail.com on 13 Nov 2012 at 8:25

[GoogleCodeExporter]

r1694 takes a stab at fixing this issue.

An incoming response is now passed automatically to the application, even if 
the request isn't completely written, and independent of the status code of the 
response.
The application gets a new API to check in the response handler if the request 
was completely or not. When it's done with the response, the request and its 
buckets will be destroyed.

Original comment by lieven.govaerts@gmail.com on 17 Nov 2012 at 8:41

[GoogleCodeExporter]

change title.

Original comment by lieven.govaerts@gmail.com on 18 Nov 2012 at 3:39

• Changed title: Serf doesn't handle an incoming 408 Request Timeout

[GoogleCodeExporter]

r1694 and r1696 implement the new mechanism, an incoming response that's 
received before the request was completely written on the socket will now be 
passed to the application.

Original comment by lieven.govaerts@gmail.com on 3 Jan 2013 at 7:45

• Changed state: Fixed