[Chicken-users] http egg and closed persistent connections

[Drew Hess]

Hi,

As you can see, I'm continuing to hack on the http egg. I've got one
more patch coming after this one. This latest one addreses some
confusing behavior in http:send-request that took me awhile to figure
out.

Some HTTP servers will drop a connection even when they've indicated
it'll be persistent. For example, the nytimes.com web servers send a
Connection: keep-alive header when the client sends an HTTP/1.0 request,
but drops the connection after a single reply, anyway. http:send-request
sees the keep-alive header and places the connection into the persistent
connection pool. The next time http:send-request connects to
nytimes.com, it'll re-use that connection if the user hasn't passed
explicit in and out port parameters, but because the connection is
closed, http:send-request's header variable gets EOF. Currently,
http:send-request doesn't detect this condition, and it returns an eof
object as the header value.

(It'd be nice if this behavior were documented in the wiki, by the
way. The documentation says that the header value returned by
http:send-request is always a string.)

Programs that use http:send-request but ignore the in and out port
values returned by http:send-request (e.g, when using the HTTP HEAD
method) have no defense against this condition except _not_ to ignore
in and out, to close them explicitly when http:send-request returns
EOF, and then to re-try the request. That's a bunch of special-case
code for a client app that just wants to send requests and doesn't
care or even know about persistent connections.

I've attached a patch for http-client that tries to find a
happy-medium. Here's the relevant portion of the patched
http:send-request:

             (let* ([header (read-line i (http:read-line-limit))]
                    [a (http:read-request-attributes i)])
               (cond ((and in out))
                     ((eof-object? header)
                      (signal (make-composite-condition
                                (make-property-condition
                                 'exn
                                 'location 'http:send-request
                                 'message "Server closed connection 
unexpectedly")
                                (make-property-condition
                                 'unexpected-eof))))
                     ((and (string? header)
                           (is-keep-alive? header a))
                      (or (is-connected? id) (add-connection! id i o)))
                     (else
                      (set-finalizer! i close-input-port)
                      (set-finalizer! o close-output-port) ) )
               (values header a i o)))
           (exn (exn i/o net)
                (cond ((and in out) (set! in #f) (set! out #f) (retry))
                      ((is-connected? id) (close-connection! id) (retry))
                      (else (signal exn))))
           (exn (exn unexpected-eof)
                (cond ((is-connected? id) ; a persistent connection closed, 
re-open it
                       (close-connection! id)
                       (retry))
                      (else (signal exn))))))))))

The ((eof-object? header) ...) cond clause and the unexpected-eof
condition case are the new bits. With this patch, the EOF behavior is
as follows:

* If in and out were specified by the caller, http:send-request
behaves like it used to. It's the caller's responsibility to handle
the EOF header condition and close in and out manually. This is the
current behavior. Programs that explicitly re-use connections won't
break, they'll still be informed when their persistent connections
drop, etc.

* Otherwise, if header is EOF, signal an (exn unexpected-eof) condition.
Then, in the condition-case handler for this exception, check to see
whether this connection was gotten from the connection pool. If so,
close it and retry the request: this behavior obviates the need for a
simple client to handle EOF conditions when http:send-request is slyly
using persistent connections behind its back. Otherwise, the connection
was not gotten from the persistent connection pool, this is a new
connection, and the server is simply misbehaving. This case is clearly a
protocol error, so let the program deal with it.

A more backward-compatible way to deal with this error condition would
be to signal (exn i/o net), so that programs that are already
handling that exception will handle this new one, as well.

A cleaner approach that eliminates signaling and handling conditions in
the same lexical scope would be to replace the named-let retry with
call/cc, eliminate the (exn (exn unexpected-eof) ...) condition case and
have the ((eof-object? header) ...) cond clause handle the entire
problem. Its behavior would be the same: if the connection came from the
pool, close it and retry by invoking the continuation, otherwise signal
an exception that will miss the (exn i/o net) handler. But that's a
bigger change than I want to make to a library with so many
corner-cases, and it obviates the backward-compatible compromise I
mentioned in the previous paragraph.

thanks!
d

p.s. All of the patches I'm sending are diffed against the current SVN
trunk for the chicken-3 version of the http egg. None of the patches
depends on any of the others. I can send a cumulative patch, too, if
whoever's maintaining the egg wants to apply them all and it's easier
that way.

diff --git a/http-client.scm b/http-client.scm
index 7f81eb2..401cfb7 100644
--- a/http-client.scm
+++ b/http-client.scm
@@ -243,6 +243,14 @@ EOF
              (let* ([header (read-line i (http:read-line-limit))]
                     [a (http:read-request-attributes i)])
                (cond ((and in out))
+                     ((eof-object? header)
+                      (signal (make-composite-condition
+                                (make-property-condition
+                                 'exn
+                                 'location 'http:send-request
+                                 'message "Server closed connection 
unexpectedly")
+                                (make-property-condition
+                                 'unexpected-eof))))
                      ((and (string? header)
                            (is-keep-alive? header a))
                       (or (is-connected? id) (add-connection! id i o)))
@@ -253,6 +261,11 @@ EOF
            (exn (exn i/o net)
                 (cond ((and in out) (set! in #f) (set! out #f) (retry))
                       ((is-connected? id) (close-connection! id) (retry))
+                      (else (signal exn))))
+           (exn (exn unexpected-eof)
+                (cond ((is-connected? id) ; a persistent connection closed, 
re-open it
+                       (close-connection! id)
+                       (retry))
                       (else (signal exn))))))))))
 
 (define (http:read-body a i)

pgpZ6KKkK2HBu.pgp
Description: PGP signature