bug#13857: Unhandled case in module/web/response.scm

bug#13857: Unhandled case in module/web/response.scm

[Jason Earl]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]


response.scm does not seem to handle the case where the server does not
specify a content length.  Here's a minimal example that should work,
but doesn't:

--8<---------------cut here---------------start------------->8---
#!/usr/local/bin/guile -s
!#

(use-modules (srfi srfi-8)
             ((web uri)    #:select (string->uri))
             ((web client) #:select (http-get)))

(receive (res-headers res-body)
    (http-get (string->uri "http://www.blogger.com/feeds/4777343509834060826/posts/default"))
  (display res-body)
  (newline))
--8<---------------cut here---------------end--------------->8---

Now the reason that I started experimenting with guile in the first
place was that I wanted to learn more about scheme, and fixing this
seemed like a good opportunity at a practical application of my basic
scheme skills.

So I did a little debugging and created this little patch that fixes
this issue.


[-- Attachment #2: 0001-Handle-case-where-server-does-not-specify-a-Content-.patch --]
[-- Type: text/x-diff, Size: 2278 bytes --]

From 74af03051fec22e01376f3f4597eb8de2dab87b5 Mon Sep 17 00:00:00 2001
From: Jason Douglas Earl <jearl@notengoamigos.org>
Date: Fri, 1 Mar 2013 23:58:41 -0700
Subject: [PATCH] Handle case where server does not specify a Content-Length.

* module/web/response.scm (response-body-port,
  make-undelimited-input-port): The server does not have to return a
  content length.  Handle that case.
---
 module/web/response.scm |   28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/module/web/response.scm b/module/web/response.scm
index 7e14f4d..33dcf1e 100644
--- a/module/web/response.scm
+++ b/module/web/response.scm
@@ -264,6 +264,26 @@ closes PORT, unless KEEP-ALIVE? is true."
 
   (make-custom-binary-input-port "delimited input port" read! #f #f close))
 
+(define (make-undelimited-input-port port keep-alive?)
+  "Return an input port that reads from PORT, until EOF.  Closing the
+returned port closes PORT, unless KEEP-ALIVE? is true."
+  (define bytes-read 0)
+
+  (define (read! bv start count)
+    (let ((ret (get-bytevector-n! port bv start count)))
+      (if (eof-object? ret)
+	  0
+          (begin
+            (set! bytes-read (+ bytes-read ret))
+	    ret))))
+
+  (define close
+    (and (not keep-alive?)
+         (lambda ()
+           (close port))))
+
+  (make-custom-binary-input-port "undelimited input port" read! #f #f close))
+
 (define* (response-body-port r #:key (decode? #t) (keep-alive? #t))
   "Return an input port from which the body of R can be read.  The
 encoding of the returned port is set according to R's ‘content-type’
@@ -277,9 +297,11 @@ response port."
         (make-chunked-input-port (response-port r)
                                  #:keep-alive? keep-alive?)
         (let ((len (response-content-length r)))
-          (and len
-               (make-delimited-input-port (response-port r)
-                                          len keep-alive?)))))
+          (if len
+	      (make-delimited-input-port (response-port r)
+					 len keep-alive?)
+	      (make-undelimited-input-port (response-port r)
+					   keep-alive?)))))
 
   (when (and decode? port)
     (match (response-content-type r)
-- 
1.7.10.4


[-- Attachment #3: Type: text/plain, Size: 960 bytes --]


With that patch my little test program works.

Now, please forgive my ignorance.  I probably misunderstand what
"delimited" means in this context, and I am probably using it
incorrectly.  I would be shocked if this even slightly resembles how
this should be fixed.  I have been lurking on the guile mailing lists
for a few months and I don't understand half of what you guys are
talking about (which is actually why I am so keen to play with guile).
Sharing this patch seemed to be the easiest way to explain what is
happening.

I am not even going to pretend that I have spent a great deal of time
reading the HTTP 1.1 protocol specs, but it does appear that the server
does not have to return a Content-Length.  I certainly have run across
servers that don't.

Poking at this issue has been quite a bit of fun for me.  So, thanks for
all of your hard work on the system.  Now I must admit that I am
interested in seeing how (and if) this gets fixed.

Jason

bug#13857: Unhandled case in module/web/response.scm

[Daniel Hartwig]

Hello

Which version of guile are you using?  Is it from recent git?  There
are perhaps related fixes to the web modules made since January.

On 2 March 2013 15:21, Jason Earl <jearl@notengoamigos.org> wrote:
>
> response.scm does not seem to handle the case where the server does not
> specify a content length.  Here's a minimal example that should work,
> but doesn't:

For non-chunked responses, Content-Length is _almost_ always present.

>
> --8<---------------cut here---------------start------------->8---
> #!/usr/local/bin/guile -s
> !#
>
> (use-modules (srfi srfi-8)
>              ((web uri)    #:select (string->uri))
>              ((web client) #:select (http-get)))
>
> (receive (res-headers res-body)
>     (http-get (string->uri "http://www.blogger.com/feeds/4777343509834060826/posts/default"))
>   (display res-body)
>   (newline))
> --8<---------------cut here---------------end--------------->8---

On my testing, this server is using chunked transfer encoding in the
response, and your patch should have no effect on that?

>
> Now the reason that I started experimenting with guile in the first
> place was that I wanted to learn more about scheme, and fixing this
> seemed like a good opportunity at a practical application of my basic
> scheme skills.
>
> So I did a little debugging and created this little patch that fixes
> this issue.
>
>
>
> With that patch my little test program works.
>
> Now, please forgive my ignorance.  I probably misunderstand what
> "delimited" means in this context, and I am probably using it
> incorrectly.  I would be shocked if this even slightly resembles how
> this should be fixed.  I have been lurking on the guile mailing lists
> for a few months and I don't understand half of what you guys are
> talking about (which is actually why I am so keen to play with guile).
> Sharing this patch seemed to be the easiest way to explain what is
> happening.
>
> I am not even going to pretend that I have spent a great deal of time
> reading the HTTP 1.1 protocol specs, but it does appear that the server
> does not have to return a Content-Length.  I certainly have run across
> servers that don't.
>
> Poking at this issue has been quite a bit of fun for me.  So, thanks for
> all of your hard work on the system.  Now I must admit that I am
> interested in seeing how (and if) this gets fixed.
>
> Jason
>

Your undelimited port has only one feature on top of a regular port:
handle #:keep-alive?.  Note that this HTTP option is not usable unless
the response includes a Content-Length header :-).

Regards

bug#13857: Unhandled case in module/web/response.scm

[Jason Earl]

On Sat, Mar 02 2013, Daniel Hartwig wrote:

> Hello
>
> Which version of guile are you using?  Is it from recent git?  There
> are perhaps related fixes to the web modules made since January.

Dang it.  I knew that there was stuff that I had forgotten to mention.
I am using the head of the stable-2.0 branch from git.  I would be happy
to use the master branch if it makes a difference, but I would have to
update libgc on the machines that I hack on.  However, the diff between
the web module in the stable-2.0 branch and master is pretty small.  I
am pretty confident that it would fail in a similar manner.

I actually have had this problem since before January, and I switched to
using guile straight out of git because I was hoping the fixes that Andy
made would solve my problem.  I considered filing a bug then, but I sort
of wanted to learn how to use the guile debugging tools.

> On 2 March 2013 15:21, Jason Earl <jearl@notengoamigos.org> wrote:
>>
>> response.scm does not seem to handle the case where the server does not
>> specify a content length.  Here's a minimal example that should work,
>> but doesn't:
>
> For non-chunked responses, Content-Length is _almost_ always present.
>
>>
>> --8<---------------cut here---------------start------------->8---
>> #!/usr/local/bin/guile -s
>> !#
>>
>> (use-modules (srfi srfi-8)
>>              ((web uri)    #:select (string->uri))
>>              ((web client) #:select (http-get)))
>>
>> (receive (res-headers res-body)
>>     (http-get (string->uri "http://www.blogger.com/feeds/4777343509834060826/posts/default"))
>>   (display res-body)
>>   (newline))
>> --8<---------------cut here---------------end--------------->8---
>
> On my testing, this server is using chunked transfer encoding in the
> response, and your patch should have no effect on that?

Did you actually try using guile as the web client?  I am just asking
because when I tested using Firefox with live http headers I would get
different headers than when I used guile.

Here's the relevant part of the trace I get when using the head of
stable-2.0:

--8<---------------cut here---------------start------------->8---
trace: |  |  (read-response-body #<<response> version: (1 . 1) code: 200 reason-phrase: …>)
trace: |  |  |  (response-body-port #<<response> version: (1 . 1) code: 200 reason-phra…> …)
trace: |  |  |  |  (response-transfer-encoding #<<response> version: (1 . 1) code: 200 r…>)
trace: |  |  |  |  |  (assq transfer-encoding ((p3p . "CP=\"This is not a P3P policy…") …))
trace: |  |  |  |  |  #f
trace: |  |  |  |  ()
trace: |  |  |  |  (member (chunked) ())
trace: |  |  |  |  #f
trace: |  |  |  |  (response-content-length #<<response> version: (1 . 1) code: 200 reas…>)
trace: |  |  |  |  |  (assq content-length ((p3p . "CP=\"This is not a P3P policy! S…") …))
trace: |  |  |  |  |  #f
trace: |  |  |  |  #f
trace: |  |  |  #f
trace: |  |  |  (and=> #f #<procedure get-bytevector-all (_)>)
trace: |  |  |  #f
trace: |  |  |  (eof-object? #f)
trace: |  |  |  #f
trace: |  |  #f
--8<---------------cut here---------------end--------------->8---

And here's the headers that I get when I connect with guile:

--8<---------------cut here---------------start------------->8---
#<<response> version: (1 . 1) code: 200 reason-phrase: "OK" headers: ((p3p . "CP=\"This is not a P3P policy! See http://www.google.com/support/accounts/bin/answer.py?hl=en&answer=151657 for more info.\"") (content-type application/atom+xml (charset . "UTF-8")) (expires . #<date nanosecond: 0 second: 40 minute: 50 hour: 4 day: 3 month: 3 year: 2013 zone-offset: 0>) (date . #<date nanosecond: 0 second: 40 minute: 50 hour: 4 day: 3 month: 3 year: 2013 zone-offset: 0>) (cache-control private (max-age . 0)) (last-modified . #<date nanosecond: 0 second: 43 minute: 58 hour: 23 day: 19 month: 2 year: 2013 zone-offset: 0>) (etag "AkQGQnoyfil7ImA9WhBSE0w." . #f) (x-content-type-options . "nosniff") (x-xss-protection . "1; mode=block") (server . "GSE") (connection close)) port: #<closed: file 0>>
--8<---------------cut here---------------end--------------->8---

>> Now the reason that I started experimenting with guile in the first
>> place was that I wanted to learn more about scheme, and fixing this
>> seemed like a good opportunity at a practical application of my basic
>> scheme skills.
>>
>> So I did a little debugging and created this little patch that fixes
>> this issue.

[...]

>> Poking at this issue has been quite a bit of fun for me.  So, thanks
>> for all of your hard work on the system.  Now I must admit that I am
>> interested in seeing how (and if) this gets fixed.
>>
>> Jason
>>
>
> Your undelimited port has only one feature on top of a regular port:
> handle #:keep-alive?.  Note that this HTTP option is not usable unless
> the response includes a Content-Length header :-).

Apparently I did not make it clear enough how green I was :).

Seriously though, I appreciate you taking the time to look into this for
me.  If my little test program works for you with *any* version of guile
that you happen to have lying around then there is probably something
wrong at my end.  I would like to think that I have uncovered an actual
bug, but I could easily be wrong.  I tested this on four different
machines, but they are all similar enough that it could still easily be
some configuration issue I have on my end.

If you have already tried the example program that I sent and you get
something besides #f then I am truly sorry to have wasted your precious
time.  If that is the case then I would appreciate hearing about it, of
course.

Jason

bug#13857: Unhandled case in module/web/response.scm

[Jason Earl]

On Sat, Mar 02 2013, Daniel Hartwig wrote:

> Hello
>
> Which version of guile are you using?  Is it from recent git?  There
> are perhaps related fixes to the web modules made since January.

Dang it.  I knew that there was stuff that I had forgotten to mention.
I am using the head of the stable-2.0 branch from git.  I would be happy
to use the master branch if it makes a difference, but I would have to
update libgc on the machines that I hack on.  However, the diff between
the web module in the stable-2.0 branch and master is pretty small.  I
am pretty confident that it would fail in a similar manner.

I actually have had this problem since before January, and I switched to
using guile straight out of git because I was hoping the fixes that Andy
made would solve my problem.  I considered filing a bug then, but I sort
of wanted to learn how to use the guile debugging tools.

> On 2 March 2013 15:21, Jason Earl <jearl@notengoamigos.org> wrote:
>>
>> response.scm does not seem to handle the case where the server does not
>> specify a content length.  Here's a minimal example that should work,
>> but doesn't:
>
> For non-chunked responses, Content-Length is _almost_ always present.
>
>>
>> --8<---------------cut here---------------start------------->8---
>> #!/usr/local/bin/guile -s
>> !#
>>
>> (use-modules (srfi srfi-8)
>>              ((web uri)    #:select (string->uri))
>>              ((web client) #:select (http-get)))
>>
>> (receive (res-headers res-body)
>>     (http-get (string->uri "http://www.blogger.com/feeds/4777343509834060826/posts/default"))
>>   (display res-body)
>>   (newline))
>> --8<---------------cut here---------------end--------------->8---
>
> On my testing, this server is using chunked transfer encoding in the
> response, and your patch should have no effect on that?

Did you actually try using guile as the web client?  I am just asking
because when I tested using Firefox with live http headers I would get
different headers than when I used guile.

Here's the relevant part of the trace I get when using the head of
stable-2.0:

--8<---------------cut here---------------start------------->8---
trace: |  |  (read-response-body #<<response> version: (1 . 1) code: 200 reason-phrase: …>)
trace: |  |  |  (response-body-port #<<response> version: (1 . 1) code: 200 reason-phra…> …)
trace: |  |  |  |  (response-transfer-encoding #<<response> version: (1 . 1) code: 200 r…>)
trace: |  |  |  |  |  (assq transfer-encoding ((p3p . "CP=\"This is not a P3P policy…") …))
trace: |  |  |  |  |  #f
trace: |  |  |  |  ()
trace: |  |  |  |  (member (chunked) ())
trace: |  |  |  |  #f
trace: |  |  |  |  (response-content-length #<<response> version: (1 . 1) code: 200 reas…>)
trace: |  |  |  |  |  (assq content-length ((p3p . "CP=\"This is not a P3P policy! S…") …))
trace: |  |  |  |  |  #f
trace: |  |  |  |  #f
trace: |  |  |  #f
trace: |  |  |  (and=> #f #<procedure get-bytevector-all (_)>)
trace: |  |  |  #f
trace: |  |  |  (eof-object? #f)
trace: |  |  |  #f
trace: |  |  #f
--8<---------------cut here---------------end--------------->8---

And here's the headers that I get when I connect with guile:

--8<---------------cut here---------------start------------->8---
#<<response> version: (1 . 1) code: 200 reason-phrase: "OK" headers: ((p3p . "CP=\"This is not a P3P policy! See http://www.google.com/support/accounts/bin/answer.py?hl=en&answer=151657 for more info.\"") (content-type application/atom+xml (charset . "UTF-8")) (expires . #<date nanosecond: 0 second: 40 minute: 50 hour: 4 day: 3 month: 3 year: 2013 zone-offset: 0>) (date . #<date nanosecond: 0 second: 40 minute: 50 hour: 4 day: 3 month: 3 year: 2013 zone-offset: 0>) (cache-control private (max-age . 0)) (last-modified . #<date nanosecond: 0 second: 43 minute: 58 hour: 23 day: 19 month: 2 year: 2013 zone-offset: 0>) (etag "AkQGQnoyfil7ImA9WhBSE0w." . #f) (x-content-type-options . "nosniff") (x-xss-protection . "1; mode=block") (server . "GSE") (connection close)) port: #<closed: file 0>>
--8<---------------cut here---------------end--------------->8---

>> Now the reason that I started experimenting with guile in the first
>> place was that I wanted to learn more about scheme, and fixing this
>> seemed like a good opportunity at a practical application of my basic
>> scheme skills.
>>
>> So I did a little debugging and created this little patch that fixes
>> this issue.

[...]

>> Poking at this issue has been quite a bit of fun for me.  So, thanks
>> for all of your hard work on the system.  Now I must admit that I am
>> interested in seeing how (and if) this gets fixed.
>>
>> Jason
>>
>
> Your undelimited port has only one feature on top of a regular port:
> handle #:keep-alive?.  Note that this HTTP option is not usable unless
> the response includes a Content-Length header :-).

Apparently I did not make it clear enough how green I was :).

Seriously though, I appreciate you taking the time to look into this for
me.  If my little test program works for you with *any* version of guile
that you happen to have lying around then there is probably something
wrong at my end.  I would like to think that I have uncovered an actual
bug, but I could easily be wrong.  I tested this on four different
machines, but they are all similar enough that it could still easily be
some configuration issue I have on my end.

If you have already tried the example program that I sent and you get
something besides #f then I am truly sorry to have wasted your precious
time.  If that is the case then I would appreciate hearing about it, of
course.

Jason

bug#13857: Unhandled case in module/web/response.scm

[Daniel Hartwig]

On 3 March 2013 12:55, Jason Earl <jearl@notengoamigos.org> wrote:
> On Sat, Mar 02 2013, Daniel Hartwig wrote:
>
>> Hello
>>
>> Which version of guile are you using?  Is it from recent git?  There
>> are perhaps related fixes to the web modules made since January.
>
> Dang it.  I knew that there was stuff that I had forgotten to mention.
> I am using the head of the stable-2.0 branch from git.  I would be happy
> to use the master branch if it makes a difference, but I would have to
> update libgc on the machines that I hack on.  However, the diff between
> the web module in the stable-2.0 branch and master is pretty small.  I
> am pretty confident that it would fail in a similar manner.

Right, using master wouldn't help, but I'm glad to learn that you are
on stable-2.0.

For future reference, when making a report like this it helps to
include the release version or git commit id you are using, at least a
note to say something like “recent stable-2.0”.

> I actually have had this problem since before January, and I switched to
> using guile straight out of git because I was hoping the fixes that Andy
> made would solve my problem.

A prudent first step.

>> On 2 March 2013 15:21, Jason Earl <jearl@notengoamigos.org> wrote:
>>>
>>> response.scm does not seem to handle the case where the server does not
>>> specify a content length.  Here's a minimal example that should work,
>>> but doesn't:
>>
>> For non-chunked responses, Content-Length is _almost_ always present.
>>
>>>
>>> --8<---------------cut here---------------start------------->8---
>>> #!/usr/local/bin/guile -s
>>> !#
>>>
>>> (use-modules (srfi srfi-8)
>>>              ((web uri)    #:select (string->uri))
>>>              ((web client) #:select (http-get)))
>>>
>>> (receive (res-headers res-body)
>>>     (http-get (string->uri "http://www.blogger.com/feeds/4777343509834060826/posts/default"))
>>>   (display res-body)
>>>   (newline))
>>> --8<---------------cut here---------------end--------------->8---
>>
>> On my testing, this server is using chunked transfer encoding in the
>> response, and your patch should have no effect on that?
>
> Did you actually try using guile as the web client?  I am just asking
> because when I tested using Firefox with live http headers I would get
> different headers than when I used guile.

No.  At the time I was not at a station with recent stable-2.0 ready
to test properly, I only performed a manual check of the headers to
explore possabilities and, having received the chunked
transfer-encoding, presumed the server was always using that.  This
now appears more likely due to the proxy active qon my connection.

It is anyway clear that ‘response-body-port’ is missing the case where
a content-length header is not present and the body is terminated by
the server closing the port.  Some additional care needs to be taken,
e.g. ‘#:keep-alive?’ is incompatible with missing content-length.
Depending on how ‘response-body-port’ is used, I will have to consider
whether to signal an error or do something else in that case.

Thanks for reporting this and hopefully it can be fixed for the next
point release.  As I am currently somewhat involved with the web
modules and related RFCs it is something else I can work on.

>
> Here's the relevant part of the trace I get when using the head of
> stable-2.0:
>
> --8<---------------cut here---------------start------------->8---
> trace: |  |  (read-response-body #<<response> version: (1 . 1) code: 200 reason-phrase: …>)
> trace: |  |  |  (response-body-port #<<response> version: (1 . 1) code: 200 reason-phra…> …)
> trace: |  |  |  |  (response-transfer-encoding #<<response> version: (1 . 1) code: 200 r…>)
> trace: |  |  |  |  |  (assq transfer-encoding ((p3p . "CP=\"This is not a P3P policy…") …))
> trace: |  |  |  |  |  #f
> trace: |  |  |  |  ()
> trace: |  |  |  |  (member (chunked) ())
> trace: |  |  |  |  #f
> trace: |  |  |  |  (response-content-length #<<response> version: (1 . 1) code: 200 reas…>)
> trace: |  |  |  |  |  (assq content-length ((p3p . "CP=\"This is not a P3P policy! S…") …))
> trace: |  |  |  |  |  #f
> trace: |  |  |  |  #f
> trace: |  |  |  #f
> trace: |  |  |  (and=> #f #<procedure get-bytevector-all (_)>)
> trace: |  |  |  #f
> trace: |  |  |  (eof-object? #f)
> trace: |  |  |  #f
> trace: |  |  #f
> --8<---------------cut here---------------end--------------->8---
>
> And here's the headers that I get when I connect with guile:
>
> --8<---------------cut here---------------start------------->8---
> #<<response> version: (1 . 1) code: 200 reason-phrase: "OK" headers: ((p3p . "CP=\"This is not a P3P policy! See http://www.google.com/support/accounts/bin/answer.py?hl=en&answer=151657 for more info.\"") (content-type application/atom+xml (charset . "UTF-8")) (expires . #<date nanosecond: 0 second: 40 minute: 50 hour: 4 day: 3 month: 3 year: 2013 zone-offset: 0>) (date . #<date nanosecond: 0 second: 40 minute: 50 hour: 4 day: 3 month: 3 year: 2013 zone-offset: 0>) (cache-control private (max-age . 0)) (last-modified . #<date nanosecond: 0 second: 43 minute: 58 hour: 23 day: 19 month: 2 year: 2013 zone-offset: 0>) (etag "AkQGQnoyfil7ImA9WhBSE0w." . #f) (x-content-type-options . "nosniff") (x-xss-protection . "1; mode=block") (server . "GSE") (connection close)) port: #<closed: file 0>>
> --8<---------------cut here---------------end--------------->8---

Right.  I guess this is when you manually perform the steps of
‘http-get’, correct?  When I do that, I get a content-encoding header
with ‘(chunked)’, but such a header is *not* present when I use
‘http-get’.  I suspect this is due to idiosyncrasies of my connection.

>
>>> Now the reason that I started experimenting with guile in the first
>>> place was that I wanted to learn more about scheme, and fixing this
>>> seemed like a good opportunity at a practical application of my basic
>>> scheme skills.
>>>
>>> So I did a little debugging and created this little patch that fixes
>>> this issue.
>
> [...]
>
>>> Poking at this issue has been quite a bit of fun for me.  So, thanks
>>> for all of your hard work on the system.  Now I must admit that I am
>>> interested in seeing how (and if) this gets fixed.
>>>
>>> Jason
>>>
>>
>> Your undelimited port has only one feature on top of a regular port:
>> handle #:keep-alive?.  Note that this HTTP option is not usable unless
>> the response includes a Content-Length header :-).
>
> Apparently I did not make it clear enough how green I was :).

No, that was fairly clear.  The above comment is a hint that we can
just use ‘(response-port r)’ rather than the undelimited port wrapper,
however some consideration needs to be given to the general usage of
‘response-body-port’ and, in particular, implications of
‘#:keep-alive?’.

Regards

bug#13857: Unhandled case in module/web/response.scm

[Andy Wingo]

On Sat 09 Mar 2013 02:27, Daniel Hartwig <mandyke@gmail.com> writes:

> It is anyway clear that ‘response-body-port’ is missing the case where
> a content-length header is not present and the body is terminated by
> the server closing the port.  Some additional care needs to be taken,
> e.g. ‘#:keep-alive?’ is incompatible with missing content-length.
> Depending on how ‘response-body-port’ is used, I will have to consider
> whether to signal an error or do something else in that case.
>
> Thanks for reporting this and hopefully it can be fixed for the next
> point release.  As I am currently somewhat involved with the web
> modules and related RFCs it is something else I can work on.

Something like this?

--- a/module/web/response.scm
+++ b/module/web/response.scm
@@ -273,13 +273,26 @@ body is available.
 When KEEP-ALIVE? is #f, closing the returned port also closes R's
 response port."
   (define port
-    (if (member '(chunked) (response-transfer-encoding r))
-        (make-chunked-input-port (response-port r)
-                                 #:keep-alive? keep-alive?)
-        (let ((len (response-content-length r)))
-          (and len
-               (make-delimited-input-port (response-port r)
-                                          len keep-alive?)))))
+    (cond
+     ((member '(chunked) (response-transfer-encoding r))
+      (make-chunked-input-port (response-port r)
+                               #:keep-alive? keep-alive?))
+     ((response-content-length r)
+      => (lambda (len)
+           (make-delimited-input-port (response-port r)
+                                      len keep-alive?)))
+     ((response-must-not-include-body? r)
+      #f)
+     ((or (memq 'close (response-connection r))
+          (and (equal? (response-version r) '(1 . 0))
+               (not (memq 'keep-alive (response-connection r)))))
+      port)
+     (else
+      ;; Here we have a message with no transfer encoding, no
+      ;; content-length, and a response that won't necessarily be closed
+      ;; by the server.  Not much we can do; assume that the client
+      ;; knows how to handle it.
+      port)))
 
   (when (and decode? port)
     (match (response-content-type r)

Andy
-- 
http://wingolog.org/

bug#13857: Unhandled case in module/web/response.scm

[Jason Earl]

On Sat, Mar 09 2013, Andy Wingo wrote:

> On Sat 09 Mar 2013 02:27, Daniel Hartwig <mandyke@gmail.com> writes:
>
>> It is anyway clear that ‘response-body-port’ is missing the case where
>> a content-length header is not present and the body is terminated by
>> the server closing the port.  Some additional care needs to be taken,
>> e.g. ‘#:keep-alive?’ is incompatible with missing content-length.
>> Depending on how ‘response-body-port’ is used, I will have to consider
>> whether to signal an error or do something else in that case.
>>
>> Thanks for reporting this and hopefully it can be fixed for the next
>> point release.  As I am currently somewhat involved with the web
>> modules and related RFCs it is something else I can work on.
>
> Something like this?
>
> --- a/module/web/response.scm
> +++ b/module/web/response.scm
> @@ -273,13 +273,26 @@ body is available.
>  When KEEP-ALIVE? is #f, closing the returned port also closes R's
>  response port."
>    (define port
> -    (if (member '(chunked) (response-transfer-encoding r))
> -        (make-chunked-input-port (response-port r)
> -                                 #:keep-alive? keep-alive?)
> -        (let ((len (response-content-length r)))
> -          (and len
> -               (make-delimited-input-port (response-port r)
> -                                          len keep-alive?)))))
> +    (cond
> +     ((member '(chunked) (response-transfer-encoding r))
> +      (make-chunked-input-port (response-port r)
> +                               #:keep-alive? keep-alive?))
> +     ((response-content-length r)
> +      => (lambda (len)
> +           (make-delimited-input-port (response-port r)
> +                                      len keep-alive?)))
> +     ((response-must-not-include-body? r)
> +      #f)
> +     ((or (memq 'close (response-connection r))
> +          (and (equal? (response-version r) '(1 . 0))
> +               (not (memq 'keep-alive (response-connection r)))))
> +      port)
> +     (else
> +      ;; Here we have a message with no transfer encoding, no
> +      ;; content-length, and a response that won't necessarily be closed
> +      ;; by the server.  Not much we can do; assume that the client
> +      ;; knows how to handle it.
> +      port)))
>  
>    (when (and decode? port)
>      (match (response-content-type r)
>
> Andy

That did not work for me.  My little test program crashes with the
following backtrace:

--8<---------------cut here---------------start------------->8---
Backtrace:
In ice-9/boot-9.scm:
 157: 7 [catch #t #<catch-closure a1175e0> ...]
In unknown file:
   ?: 6 [apply-smob/1 #<catch-closure a1175e0>]
In ice-9/boot-9.scm:
  63: 5 [call-with-prompt prompt0 ...]
In ice-9/eval.scm:
 432: 4 [eval # #]
In /home/jearl/projects/scratch/testing.scm:
  12: 3 [main ("./testing.scm")]
In web/client.scm:
 230: 2 [request # # #f ...]
In web/response.scm:
 310: 1 [read-response-body #]
In unknown file:
   ?: 0 [get-bytevector-all #<unspecified>]
--8<---------------cut here---------------end--------------->8---

I hesitate to comment further, as I really do *not* know what I am
doing. However, is it possible that you mean something like this?

--8<---------------cut here---------------start------------->8---
diff --git a/module/web/response.scm b/module/web/response.scm
index 7e14f4d..3f97dff 100644
--- a/module/web/response.scm
+++ b/module/web/response.scm
@@ -273,13 +273,26 @@ body is available.
 When KEEP-ALIVE? is #f, closing the returned port also closes R's
 response port."
   (define port
-    (if (member '(chunked) (response-transfer-encoding r))
-        (make-chunked-input-port (response-port r)
-                                 #:keep-alive? keep-alive?)
-        (let ((len (response-content-length r)))
-          (and len
-               (make-delimited-input-port (response-port r)
-                                          len keep-alive?)))))
+    (cond
+     ((member '(chunked) (response-transfer-encoding r))
+      (make-chunked-input-port (response-port r)
+                               #:keep-alive? keep-alive?))
+     ((response-content-length r)
+      => (lambda (len)
+           (make-delimited-input-port (response-port r)
+                                      len keep-alive?)))
+     ((response-must-not-include-body? r)
+      #f)
+     ((or (memq 'close (response-connection r))
+          (and (equal? (response-version r) '(1 . 0))
+               (not (memq 'keep-alive (response-connection r)))))
+      (response-port r))
+     (else
+      ;; Here we have a message with no transfer encoding, no
+      ;; content-length, and a response that won't necessarily be closed
+      ;; by the server.  Not much we can do; assume that the client
+      ;; knows how to handle it.
+      (response-port r))))
 
   (when (and decode? port)
     (match (response-content-type r)
--8<---------------cut here---------------end--------------->8---

That does work for me.

Thanks for taking the time to look at this.  I really feel like I am
learning a lot.

Jason Earl

bug#13857: Unhandled case in module/web/response.scm

[Daniel Hartwig]

On 10 March 2013 06:59, Jason Earl <jearl@notengoamigos.org> wrote:
> On Sat, Mar 09 2013, Andy Wingo wrote:
>
>> On Sat 09 Mar 2013 02:27, Daniel Hartwig <mandyke@gmail.com> writes:
>>
>>> It is anyway clear that ‘response-body-port’ is missing the case where
>>> a content-length header is not present and the body is terminated by
>>> the server closing the port.  Some additional care needs to be taken,
>>> e.g. ‘#:keep-alive?’ is incompatible with missing content-length.
>>> Depending on how ‘response-body-port’ is used, I will have to consider
>>> whether to signal an error or do something else in that case.
>>>
>>> Thanks for reporting this and hopefully it can be fixed for the next
>>> point release.  As I am currently somewhat involved with the web
>>> modules and related RFCs it is something else I can work on.
>>
>> Something like this?
>>
>> [patch]

Yes, more or less.  :-)

bug#13857: Unhandled case in module/web/response.scm

[Andy Wingo]

On Sat 09 Mar 2013 23:59, Jason Earl <jearl@notengoamigos.org> writes:

> That did not work for me. [I]s it possible that you mean something
> like this?

Indeed -- good catch!  Thank you for the report, patch, and testing :-)

commit 84dfde82ae8f6ec247c1c147c1e2ae50b207bad9
Author: Jason Earl <jearl@notengoamigos.org>
Date:   Sun Mar 10 19:23:31 2013 +0100

    fix response-body-port for responses without content-length
    
    * module/web/response.scm (response-body-port): Correctly handle cases
      in which EOF terminates the body.

Cheers,

Andy
-- 
http://wingolog.org/