On Thu, Nov 01, 2012 at 05:30:18PM +, Andy Bennett wrote:
> Hi,
Hi!
> When http-client receives a 304 Not Modified response it tries to
> read-string until the end of the connection. 304 Not Modified responses
> must not have a body and therefore, on keep-alive connections, the read
> hangs until it times out.
Absolutely, the current behavior is dead wrong.
> Here we present a patch that handles this case and also handles the
> similar 204 No Content reply.
>
> We don't bother to handle the similar, but different, 1xx series of
> responses because they are complicated in other, more interesting, ways.
I think this patch is handling it at the wrong level. A 304 situation
isn't really an error; it might be exactly what you're expecting if
you want to check whether something has changed. In that respect
it's similar to a 404, which is a client invocation error, so it should
be handled at the same level.
The lower-level call-with-response handler should only raise an
exception on proper *errors*, not on conditions a typical request
isn't prepared for. The higher-level call-with-input-request should
handle this. I admit, this level separation is somewhat arbitrary,
the idea being that the lower-level should act as "middleware" and
try to intelligently handle everything that can reasonably be expected
to be automatically handled.
In fact, the higher level *should* already handle 204 responses
correctly; see in the response reader it sets up, if the response class
is 204 it checks if the response has a message body for the request.
The parameter "response-has-message-body-for-request?" has a default
implementation in intarweb that looks like the following:
(lambda (resp req)
(not (or (= (response-class resp) 100)
(memv (response-code resp) '(204 304))
(eq? 'HEAD (request-method req)
This should already handle 204, 304 and the whole class of 1xx responses
as well as HEAD requests. In my opinion, the mistake in http-client is
the fact it isn't calling this parameter's procedure in non-2xx cases
before reading the response body.
Could you please give the attached patch a test? If it works I can
commit it and maybe make a new Spiffy release. I don't have a proper
test suite for http-client to test this against. I think this should
be fixed, but it's hard to test all the possible constellations of
response headers and codes possible that occur in the field (including
broken implementations). The test also moves around the parameter call
from inside where the 200 responses are handled to the generic bit to
avoid duplication.
> Thanks are due to Moritz Heidkamp for helping to read the HTTP spec.
Good work, guys; your analysis of the spec is correct (at least, to my
understanding that's how it should work).
Cheers,
Peter
--
http://sjamaan.ath.cx
--
"The process of preparing programs for a digital computer
is especially attractive, not only because it can be economically
and scientifically rewarding, but also because it can be an aesthetic
experience much like composing poetry or music."
-- Donald Knuth
Index: http-client.scm
===
--- http-client.scm (revision 27750)
+++ http-client.scm (working copy)
@@ -690,11 +690,11 @@
(lambda (response)
(let ((port (make-delimited-input-port
(response-port response)
-(header-value 'content-length (response-headers
response)
+(header-value 'content-length (response-headers
response
+ (body? ((response-has-message-body-for-request?) response req)))
(if (= 200 (response-class response)) ; Everything cool?
- (let* ((b? ((response-has-message-body-for-request?) response
req))
-(result (and b? reader (reader port
- (when b? (discard-remaining-data! #f port))
+ (let ((result (and body? reader (reader port
+ (when body? (discard-remaining-data! #f port))
result)
(http-client-error
'call-with-input-request
@@ -712,7 +712,7 @@
((500) 'server-error)
(else 'unexpected-server-response))
'response response
- 'body (read-string #f port
+ 'body (and body? (read-string #f port)
(define (with-input-from-request uri-or-request writer reader)
(call-with-input-request uri-or-request
___
Chicken-users mailing list
Chicken-users@nongnu.org
https://lists.nongnu.org/mailman/listinfo/chicken-users
