You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by Kevin Clark <ke...@gmail.com> on 2008/06/21 01:38:58 UTC
Ruby Thrift::Socket#read implementation
Figured this discussion might be useful for others, so we'll continue it here...
19:33 < kevinclark> Eridius_: is there a reason you didn't just swap
Thrift::Socket over to readpartial completely? I _think_ my
borrow implementation on BufferedTransport relies on that
(since it asks for more than could be available, and I
don't want it to blcok, but I don't want it to necessarily
rely on Socket)
19:34 < kevinclark> Eridius_: I'm finishing up a 3rd try at this merge, I'd
appreciate it if you'd review before I push. I'm going to
make a ticket for it.
19:34 < Eridius> send me an email, I can't focus on IRC right now
19:34 < kevinclark> k
19:35 < Eridius> quick comment: readpartial blocks if there's nothing to read.
read_nonblock doesn't.
19:35 * Eridius idles again
19:35 < kevinclark> but it seems to fallback to read, not read_nonblock.
--
Kevin Clark
http://glu.ttono.us
Re: Ruby Thrift::Socket#read implementation
Posted by Kevin Clark <ke...@gmail.com>.
On Tue, Jun 24, 2008 at 5:06 PM, Mark Slee <ms...@facebook.com> wrote:
> The general contract in all other language implementations is that
> read() behaves like the syscall read() -- it is not guaranteed to return
> any number of bytes. I think we should try to follow that paradigm in
> all language implementations to keep things consistent:
>
> read = try to read some number of bytes
> readAll = fail if you can't
Right, this is what we're going with.
--
Kevin Clark
http://glu.ttono.us
RE: Ruby Thrift::Socket#read implementation
Posted by Mark Slee <ms...@facebook.com>.
The general contract in all other language implementations is that
read() behaves like the syscall read() -- it is not guaranteed to return
any number of bytes. I think we should try to follow that paradigm in
all language implementations to keep things consistent:
read = try to read some number of bytes
readAll = fail if you can't
-----Original Message-----
From: Kevin Clark [mailto:kevin.clark@gmail.com]
Sent: Tuesday, June 24, 2008 4:54 PM
To: thrift-dev@incubator.apache.org
Subject: Re: Ruby Thrift::Socket#read implementation
19:49 < kevinclark> re: Thrift::Socket#read impl, I think you're
wrong. I don't think any transport is allowed to expect read to return
the right number of bytes. read takes one
argument, the _maxsize_, and read_all assures you
get all the data you ask for
19:49 < kevinclark> I think that's why I had changed read to use
readpartial
19:49 < kevinclark> I'm pretty sure I've spent a day thinking about htis
before
19:49 < Eridius|work> ahh, I forgot about read_all
19:49 < kevinclark> I guess the question is, why have read_all as a
guarenteed implementation if you expect read to always return the
right number of bytes?
19:49 < kevinclark> I think read is our readpartial
19:50 < Eridius|work> possibly. you might want to double-check all
usages of #read though
19:50 < kevinclark> I think I did that before
19:50 < kevinclark> but I'll do it again
19:50 < kevinclark> heh
19:50 < Eridius|work> if it turns out that #read is supposed to act
like #readpartial, then that's good, I can get rid of the #readpartial
and the ugly second arg
I'll follow up with final conclusions.
--
Kevin Clark
http://glu.ttono.us
Re: Ruby Thrift::Socket#read implementation
Posted by Kevin Clark <ke...@gmail.com>.
19:49 < kevinclark> re: Thrift::Socket#read impl, I think you're
wrong. I don't think any transport is allowed to expect read to return
the right number of bytes. read takes one
argument, the _maxsize_, and read_all assures you
get all the data you ask for
19:49 < kevinclark> I think that's why I had changed read to use readpartial
19:49 < kevinclark> I'm pretty sure I've spent a day thinking about htis before
19:49 < Eridius|work> ahh, I forgot about read_all
19:49 < kevinclark> I guess the question is, why have read_all as a
guarenteed implementation if you expect read to always return the
right number of bytes?
19:49 < kevinclark> I think read is our readpartial
19:50 < Eridius|work> possibly. you might want to double-check all
usages of #read though
19:50 < kevinclark> I think I did that before
19:50 < kevinclark> but I'll do it again
19:50 < kevinclark> heh
19:50 < Eridius|work> if it turns out that #read is supposed to act
like #readpartial, then that's good, I can get rid of the #readpartial
and the ugly second arg
I'll follow up with final conclusions.
--
Kevin Clark
http://glu.ttono.us
Re: Ruby Thrift::Socket#read implementation
Posted by Kevin Ballard <ke...@rapleaf.com>.
Ok, after glancing at the code, I left the default behavior using
read() because that was the pre-existing behavior, and there's code in
thrift that relies on it. For example, when the framed transport wants
data, it read()'s 4 bytes, then read()'s the number of bytes described
by those first 4 bytes. If Thrift::Socket used readpartial everywhere,
then the framed transport code would break because it expects to block
until the data it asked for is all there.
-Kevin Ballard
On Jun 20, 2008, at 4:38 PM, Kevin Clark wrote:
> Figured this discussion might be useful for others, so we'll
> continue it here...
>
> 19:33 < kevinclark> Eridius_: is there a reason you didn't just swap
> Thrift::Socket over to readpartial completely? I
> _think_ my
> borrow implementation on BufferedTransport relies
> on that
> (since it asks for more than could be available,
> and I
> don't want it to blcok, but I don't want it to
> necessarily
> rely on Socket)
> 19:34 < kevinclark> Eridius_: I'm finishing up a 3rd try at this
> merge, I'd
> appreciate it if you'd review before I push. I'm
> going to
> make a ticket for it.
> 19:34 < Eridius> send me an email, I can't focus on IRC right now
> 19:34 < kevinclark> k
> 19:35 < Eridius> quick comment: readpartial blocks if there's
> nothing to read.
> read_nonblock doesn't.
> 19:35 * Eridius idles again
> 19:35 < kevinclark> but it seems to fallback to read, not
> read_nonblock.
--
Kevin Ballard
kevin@rapleaf.com