[PATCH] read-response-body should return received data when error occcurs (V2)

[PATCH] read-response-body should return received data when error occcurs (V2)

[Nala Ginrut]

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

From 183abb7e7d649fe4a1d1799b97e6da96f51b683c Mon Sep 17 00:00:00 2001
From: NalaGinrut <NalaGinrut@gmail.com>
Date: Fri, 16 Mar 2012 13:41:34 +0800
Subject: [PATCH] read-response-body returns received data when error occurs

---
 module/web/response.scm |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/module/web/response.scm b/module/web/response.scm
index 07e1245..5c6861a 100644
--- a/module/web/response.scm
+++ b/module/web/response.scm
@@ -228,12 +228,12 @@ This is true for some response types, like those with
code 304."
   "Reads the response body from @var{r}, as a bytevector.  Returns
 @code{#f} if there was no response body."
   (let ((nbytes (response-content-length r)))
-    (and nbytes
-         (let ((bv (get-bytevector-n (response-port r) nbytes)))
-           (if (= (bytevector-length bv) nbytes)
-               bv
+    (and nbytes
+         (let* ((bv (get-bytevector-n (response-port r) nbytes)))
+           (if (eof-object? bv)
                (bad-response "EOF while reading response body: ~a bytes of
~a"
-                            (bytevector-length bv) nbytes))))))
+                            0 nbytes)
+               bv)))))

 (define (write-response-body r bv)
   "Write @var{bv}, a bytevector, to the port corresponding to the HTTP
-- 
1.7.0.4

[-- Attachment #2: Type: text/html, Size: 1662 bytes --]

Re: [PATCH] read-response-body should return received data when error occcurs (V2)

[Nala Ginrut]

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

This patch will return any data get-bytevector-n received and throw error
when get <eof>.
Actually, it's not the same feature in the old version thread
http://lists.gnu.org/archive/html/guile-devel/2012-03/msg00116.html
The old version is complicated because it catches *any* exception and
return the received data within exception.
But this new version is an easy one.
My point is, read-response-body is a low level procedure, so we shouldn't
make it complex. But our current doesn't return received data when the
received data is less than the content-length. I think it should return it,
and let the user  determine whether it's an error or continue reading.
 And if any one need a more complicated handler, it's better to write
his/her own body reader. My vote is to write a high level HTTP library to
deal with the complex situation.

[-- Attachment #2: Type: text/html, Size: 973 bytes --]

Re: [PATCH] read-response-body should return received data when error occcurs (V2)

[Nala Ginrut]

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

But our current doesn't
----------------------------
s/current/current read-response-body

Regards.

On Fri, Mar 16, 2012 at 1:54 PM, Nala Ginrut <nalaginrut@gmail.com> wrote:

> This patch will return any data get-bytevector-n received and throw error
> when get <eof>.
> Actually, it's not the same feature in the old version thread
> http://lists.gnu.org/archive/html/guile-devel/2012-03/msg00116.html
> The old version is complicated because it catches *any* exception and
> return the received data within exception.
> But this new version is an easy one.
> My point is, read-response-body is a low level procedure, so we shouldn't
> make it complex. But our current doesn't return received data when the
> received data is less than the content-length. I think it should return it,
> and let the user  determine whether it's an error or continue reading.
>  And if any one need a more complicated handler, it's better to write
> his/her own body reader. My vote is to write a high level HTTP library to
> deal with the complex situation.
>

[-- Attachment #2: Type: text/html, Size: 1420 bytes --]

Re: [PATCH] read-response-body should return received data when error occcurs (V2)

[Daniel Hartwig]

On 16 March 2012 13:54, Nala Ginrut <nalaginrut@gmail.com> wrote:
> This patch will return any data get-bytevector-n received and throw error
> when get <eof>.
> Actually, it's not the same feature in the old version thread
> http://lists.gnu.org/archive/html/guile-devel/2012-03/msg00116.html
> The old version is complicated because it catches *any* exception and return
> the received data within exception.
> But this new version is an easy one.

Yes, it looks much nicer :-)

> My point is, read-response-body is a low level procedure, so we shouldn't
> make it complex. But our current doesn't return received data when
> the received data is less than the content-length. I think it should return
> it, and let the user  determine whether it's an error or continue reading.

So r-r-b is a really a two-liner:

- read response body from port;
- make sure the complete response has been read.

This combination is the utility of the procedure: it guarentees that
the data it returns comprises the complete response body.

With your proposed change, that guarentee no longer holds.  The caller
now must perform their own checks on the response data size, making
your function effectively this:

- read response body data from port.

So what is the utility of calling a procedure to do that over, say,
reading from the port directly? [pointed out earlier in this thread]

Re: [PATCH] read-response-body should return received data when error occcurs (V2)

[Nala Ginrut]

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

Well, I saw your point. So read-response-body is defined to read the whole
body, and shouldn't return anything when it didn't get all the data.
For this reason, one should use bytevector handler to get the data on one's
own rather than read-response-body.
Well, I just thought read-response-body is the only API to get body, at
least from the manual, I can only get this conclusion. But in fact, the
worth of read-response-body is very limited. It just a simple procedure to
get a simple page or few data.
If possible, I wish the manual can add few words to notice this.

On Fri, Mar 16, 2012 at 3:32 PM, Daniel Hartwig <mandyke@gmail.com> wrote:

> On 16 March 2012 13:54, Nala Ginrut <nalaginrut@gmail.com> wrote:
> > This patch will return any data get-bytevector-n received and throw error
> > when get <eof>.
> > Actually, it's not the same feature in the old version thread
> > http://lists.gnu.org/archive/html/guile-devel/2012-03/msg00116.html
> > The old version is complicated because it catches *any* exception and
> return
> > the received data within exception.
> > But this new version is an easy one.
>
> Yes, it looks much nicer :-)
>
> > My point is, read-response-body is a low level procedure, so we shouldn't
> > make it complex. But our current doesn't return received data when
> > the received data is less than the content-length. I think it should
> return
> > it, and let the user  determine whether it's an error or continue
> reading.
>
> So r-r-b is a really a two-liner:
>
> - read response body from port;
> - make sure the complete response has been read.
>
> This combination is the utility of the procedure: it guarentees that
> the data it returns comprises the complete response body.
>
> With your proposed change, that guarentee no longer holds.  The caller
> now must perform their own checks on the response data size, making
> your function effectively this:
>
> - read response body data from port.
>
> So what is the utility of calling a procedure to do that over, say,
> reading from the port directly? [pointed out earlier in this thread]
>

[-- Attachment #2: Type: text/html, Size: 2687 bytes --]

Re: [PATCH] read-response-body should return received data when error occcurs (V2)

[Tristan Colgate]

I got this the other day with lalr.scm. My build got stuck (100% CPU
for quite a while). I ended up clearing out all old .go files and
trying again and it got past it (didn't even take that long, a few
seconds compared to a 15 minute wait).

On 16 March 2012 08:47, Nala Ginrut <nalaginrut@gmail.com> wrote:
> Well, I saw your point. So read-response-body is defined to read the whole
> body, and shouldn't return anything when it didn't get all the data.
> For this reason, one should use bytevector handler to get the data on one's
> own rather than read-response-body.
> Well, I just thought read-response-body is the only API to get body, at
> least from the manual, I can only get this conclusion. But in fact, the
> worth of read-response-body is very limited. It just a simple procedure to
> get a simple page or few data.
> If possible, I wish the manual can add few words to notice this.
>
>
> On Fri, Mar 16, 2012 at 3:32 PM, Daniel Hartwig <mandyke@gmail.com> wrote:
>>
>> On 16 March 2012 13:54, Nala Ginrut <nalaginrut@gmail.com> wrote:
>> > This patch will return any data get-bytevector-n received and throw
>> > error
>> > when get <eof>.
>> > Actually, it's not the same feature in the old version thread
>> > http://lists.gnu.org/archive/html/guile-devel/2012-03/msg00116.html
>> > The old version is complicated because it catches *any* exception and
>> > return
>> > the received data within exception.
>> > But this new version is an easy one.
>>
>> Yes, it looks much nicer :-)
>>
>> > My point is, read-response-body is a low level procedure, so we
>> > shouldn't
>> > make it complex. But our current doesn't return received data when
>> > the received data is less than the content-length. I think it should
>> > return
>> > it, and let the user  determine whether it's an error or continue
>> > reading.
>>
>> So r-r-b is a really a two-liner:
>>
>> - read response body from port;
>> - make sure the complete response has been read.
>>
>> This combination is the utility of the procedure: it guarentees that
>> the data it returns comprises the complete response body.
>>
>> With your proposed change, that guarentee no longer holds.  The caller
>> now must perform their own checks on the response data size, making
>> your function effectively this:
>>
>> - read response body data from port.
>>
>> So what is the utility of calling a procedure to do that over, say,
>> reading from the port directly? [pointed out earlier in this thread]
>
>



-- 
Tristan Colgate-McFarlane
----
  "You can get all your daily vitamins from 52 pints of guiness, and a
glass of milk"

Re: [PATCH] read-response-body should return received data when error occcurs (V2)

[Nala Ginrut]

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

Wrong thread but interesting ;-)
Could you resend it to the correct thread? Maybe helpful to people there.

Regards.

On Sat, Mar 17, 2012 at 12:27 AM, Tristan Colgate <tcolgate@gmail.com>wrote:

> I got this the other day with lalr.scm. My build got stuck (100% CPU
> for quite a while). I ended up clearing out all old .go files and
> trying again and it got past it (didn't even take that long, a few
> seconds compared to a 15 minute wait).
>
> On 16 March 2012 08:47, Nala Ginrut <nalaginrut@gmail.com> wrote:
> > Well, I saw your point. So read-response-body is defined to read the
> whole
> > body, and shouldn't return anything when it didn't get all the data.
> > For this reason, one should use bytevector handler to get the data on
> one's
> > own rather than read-response-body.
> > Well, I just thought read-response-body is the only API to get body, at
> > least from the manual, I can only get this conclusion. But in fact, the
> > worth of read-response-body is very limited. It just a simple procedure
> to
> > get a simple page or few data.
> > If possible, I wish the manual can add few words to notice this.
> >
> >
> > On Fri, Mar 16, 2012 at 3:32 PM, Daniel Hartwig <mandyke@gmail.com>
> wrote:
> >>
> >> On 16 March 2012 13:54, Nala Ginrut <nalaginrut@gmail.com> wrote:
> >> > This patch will return any data get-bytevector-n received and throw
> >> > error
> >> > when get <eof>.
> >> > Actually, it's not the same feature in the old version thread
> >> > http://lists.gnu.org/archive/html/guile-devel/2012-03/msg00116.html
> >> > The old version is complicated because it catches *any* exception and
> >> > return
> >> > the received data within exception.
> >> > But this new version is an easy one.
> >>
> >> Yes, it looks much nicer :-)
> >>
> >> > My point is, read-response-body is a low level procedure, so we
> >> > shouldn't
> >> > make it complex. But our current doesn't return received data when
> >> > the received data is less than the content-length. I think it should
> >> > return
> >> > it, and let the user  determine whether it's an error or continue
> >> > reading.
> >>
> >> So r-r-b is a really a two-liner:
> >>
> >> - read response body from port;
> >> - make sure the complete response has been read.
> >>
> >> This combination is the utility of the procedure: it guarentees that
> >> the data it returns comprises the complete response body.
> >>
> >> With your proposed change, that guarentee no longer holds.  The caller
> >> now must perform their own checks on the response data size, making
> >> your function effectively this:
> >>
> >> - read response body data from port.
> >>
> >> So what is the utility of calling a procedure to do that over, say,
> >> reading from the port directly? [pointed out earlier in this thread]
> >
> >
>
>
>
> --
> Tristan Colgate-McFarlane
> ----
>   "You can get all your daily vitamins from 52 pints of guiness, and a
> glass of milk"
>
>

[-- Attachment #2: Type: text/html, Size: 4081 bytes --]

Re: [PATCH] read-response-body should return received data when error occcurs (V2)

[Ian Price]

Nala Ginrut <nalaginrut@gmail.com> writes:

> +         (let* ((bv (get-bytevector-n (response-port r) nbytes)))
> +           (if (eof-object? bv)
>                 (bad-response "EOF while reading response body: ~a bytes of ~a"
> -                            (bytevector-length bv) nbytes))))))
> +                            0 nbytes)
> +               bv)))))

This still doesn't answer my concerns. I don't think the user should
have to be checking the length of the result themselves.

(let ((bv (get-bytevector-n (response-port r) nbytes)))
  (cond ((eof-object? bv)
         (incomplete-response ...))
        ((= (bytevector-length bv) nbytes) bv)
        (else
         (incomplete-response ...))))

Am I the only one who feels strongly about this?

-- 
Ian Price

"Programming is like pinball. The reward for doing it well is
the opportunity to do it again" - from "The Wizardy Compiled"

Re: [PATCH] read-response-body should return received data when error occcurs (V2)

[Daniel Hartwig]

On 17 March 2012 00:57, Ian Price <ianprice90@googlemail.com> wrote:
>
> This still doesn't answer my concerns. I don't think the user should
> have to be checking the length of the result themselves.
>
> (let ((bv (get-bytevector-n (response-port r) nbytes)))
>  (cond ((eof-object? bv)
>         (incomplete-response ...))
>        ((= (bytevector-length bv) nbytes) bv)
>        (else
>         (incomplete-response ...))))
>

This.

> Am I the only one who feels strongly about this?
>

No.  I guess you are still reading the rest of the thread ;-)

Re: [PATCH] read-response-body should return received data when error occcurs (V2)

[Nala Ginrut]

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

Well, to clarify, I confess I did a wrong prejudging. I thought
read-response-body is the only API to retrieve the body/data, so we should
use it anyway to get the data rather than writing other procedure to handle
it. Because the manual has only one API relate to this topic.
But I was *wrong*. This read-response-body is just a simple procedure to
get few body/data under easy circumstance. It is defined so. Anyone who
needs a more complicated operation must write his own procedure with
bytevector handlers or write a new http lib. So it's no need to change it's
behavior with any patch.

On Sat, Mar 17, 2012 at 1:25 AM, Daniel Hartwig <mandyke@gmail.com> wrote:

> On 17 March 2012 00:57, Ian Price <ianprice90@googlemail.com> wrote:
> >
> > This still doesn't answer my concerns. I don't think the user should
> > have to be checking the length of the result themselves.
> >
> > (let ((bv (get-bytevector-n (response-port r) nbytes)))
> >  (cond ((eof-object? bv)
> >         (incomplete-response ...))
> >        ((= (bytevector-length bv) nbytes) bv)
> >        (else
> >         (incomplete-response ...))))
> >
>
> This.
>
> > Am I the only one who feels strongly about this?
> >
>
> No.  I guess you are still reading the rest of the thread ;-)
>
>

[-- Attachment #2: Type: text/html, Size: 1731 bytes --]