Better handling of streams of unknown size.

[yovadia, 2012-06-14 15:14:46]

I made one suggestion about making newly computed file sizes available to read
after an upload, otherwise this looks good.

http://codereview.appspot.com/6300093/diff/1/apiclient/http.py
File apiclient/http.py (right):

http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#newcode626
apiclient/http.py:626: size = str(self.resumable_progress + len(data))
It might be useful to update self.resumable._size here, so the value would be
accessible outside of next_chunk().

[jcgregorio_google, 2012-06-14 15:41:35]

http://codereview.appspot.com/6300093/diff/1/apiclient/http.py
File apiclient/http.py (right):

http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#newcode626
apiclient/http.py:626: size = str(self.resumable_progress + len(data))
I thought about that, but it seems wrong to alter a private property of an
object that was passed into us. 

On 2012/06/14 15:14:46, yovadia wrote:
> It might be useful to update self.resumable._size here, so the value would be
> accessible outside of next_chunk().

[yovadia, 2012-06-14 15:49:50]

In that case, maybe it should happen in MediaIoBaseUpload.getbytes() ?

On Thu, Jun 14, 2012 at 8:41 AM, <jcgregorio@google.com> wrote:

>
>
http://codereview.appspot.com/**6300093/diff/1/apiclient/http.**py<http://cod...
> File apiclient/http.py (right):
>
> http://codereview.appspot.com/**6300093/diff/1/apiclient/http.**
>
py#newcode626<http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#newcode626>
> apiclient/http.py:626: size = str(self.resumable_progress + len(data))
> I thought about that, but it seems wrong to alter a private property of
> an object that was passed into us.
>
>
> On 2012/06/14 15:14:46, yovadia wrote:
>
>> It might be useful to update self.resumable._size here, so the value
>>
> would be
>
>> accessible outside of next_chunk().
>>
>
>
http://codereview.appspot.com/**6300093/<http://codereview.appspot.com/6300093/>
>

[jcgregorio_google, 2012-06-14 18:56:15]

On 2012/06/14 15:49:50, yovadia wrote:
> In that case, maybe it should happen in MediaIoBaseUpload.getbytes() ?

Then all MediaUpload objects need to remember to change their _size during
getbytes()? Seems like something that could be easily forgotten, and for not a
lot of benefit.


> 
> On Thu, Jun 14, 2012 at 8:41 AM, <mailto:jcgregorio@google.com> wrote:
> 
> >
> >
>
http://codereview.appspot.com/**6300093/diff/1/apiclient/http.**py%3Chttp://c...>
> > File apiclient/http.py (right):
> >
> > http://codereview.appspot.com/**6300093/diff/1/apiclient/http.**
> >
>
py#newcode626<http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#newcode626>
> > apiclient/http.py:626: size = str(self.resumable_progress + len(data))
> > I thought about that, but it seems wrong to alter a private property of
> > an object that was passed into us.
> >
> >
> > On 2012/06/14 15:14:46, yovadia wrote:
> >
> >> It might be useful to update self.resumable._size here, so the value
> >>
> > would be
> >
> >> accessible outside of next_chunk().
> >>
> >
> >
>
http://codereview.appspot.com/**6300093/%3Chttp://codereview.appspot.com/6300...>
> >

[nherring, 2012-06-14 19:54:53]

http://codereview.appspot.com/6300093/diff/1/apiclient/http.py
File apiclient/http.py (left):

http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#oldcode618
apiclient/http.py:618: self.resumable_progress, self.resumable.chunksize())
I don't see how MediaIoBaseUpload deals with non-blocking mode IOBase files --
you can read and get fewer bytes back than you request and still not be at EOF.
If you'd block, it returns None instead of 0. The only EOF is if you ask for n
bytes and get 0. (So if you ask for chunksize, and get a part of chunksize, you
have to ask again for the remainder and only if you get back zero are you at
EOF.)

http://docs.python.org/dev/library/io.html#io.RawIOBase.read

[jcgregorio_google, 2012-06-14 20:09:39]

There are bigger problems if the stream is non-blocking, dribbling
out what could be very small blocks of bytes would be very
inefficient. If the stream is non-blocking then reads should be
accumulated until they reach chunksize, and then sent.

I will add a comment in the docstring that only blocking streams
should be used.


On 2012/06/14 19:54:53, nherring wrote:
> http://codereview.appspot.com/6300093/diff/1/apiclient/http.py
> File apiclient/http.py (left):
> 
> http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#oldcode618
> apiclient/http.py:618: self.resumable_progress, self.resumable.chunksize())
> I don't see how MediaIoBaseUpload deals with non-blocking mode IOBase files --
> you can read and get fewer bytes back than you request and still not be at
EOF.
> If you'd block, it returns None instead of 0. The only EOF is if you ask for n
> bytes and get 0. (So if you ask for chunksize, and get a part of chunksize,
you
> have to ask again for the remainder and only if you get back zero are you at
> EOF.)
> 
> http://docs.python.org/dev/library/io.html#io.RawIOBase.read

[nherring, 2012-06-14 20:15:29]

As things stand, I think you're right. I'd love to see HttpLib2 have real
support for HTTP streams (i.e., not having the entire request in memory),
and at that point, having an ability to put things on the wire as we get
them would be great, so we're not stuck transmitting the whole chunksize
when we get that last byte. :)

On Thu, Jun 14, 2012 at 1:09 PM, <jcgregorio@google.com> wrote:

> There are bigger problems if the stream is non-blocking, dribbling
> out what could be very small blocks of bytes would be very
> inefficient. If the stream is non-blocking then reads should be
> accumulated until they reach chunksize, and then sent.
>
> I will add a comment in the docstring that only blocking streams
> should be used.
>
>
>
> On 2012/06/14 19:54:53, nherring wrote:
>
>>
http://codereview.appspot.com/**6300093/diff/1/apiclient/http.**py<http://cod...
>> File apiclient/http.py (left):
>>
>
>
> http://codereview.appspot.com/**6300093/diff/1/apiclient/http.**
>
py#oldcode618<http://codereview.appspot.com/6300093/diff/1/apiclient/http.py#oldcode618>
>
>> apiclient/http.py:618: self.resumable_progress,
>>
> self.resumable.chunksize())
>
>> I don't see how MediaIoBaseUpload deals with non-blocking mode IOBase
>>
> files --
>
>> you can read and get fewer bytes back than you request and still not
>>
> be at EOF.
>
>> If you'd block, it returns None instead of 0. The only EOF is if you
>>
> ask for n
>
>> bytes and get 0. (So if you ask for chunksize, and get a part of
>>
> chunksize, you
>
>> have to ask again for the remainder and only if you get back zero are
>>
> you at
>
>> EOF.)
>>
>
> 
http://docs.python.org/dev/**library/io.html#io.RawIOBase.**read<http://docs....
>>
>
>
>
>
http://codereview.appspot.com/**6300093/<http://codereview.appspot.com/6300093/>
>



-- 
Nathan Herring | Software Engineer | nherring@google.com | 616-466-4646

[jcgregorio_google, 2012-06-14 20:31:41]

http://codereview.appspot.com/6300093/diff/1004/apiclient/http.py
File apiclient/http.py (right):

http://codereview.appspot.com/6300093/diff/1004/apiclient/http.py#newcode307
apiclient/http.py:307: opened in blocking mode, do not use streams opened in
non-blocking mode.
Updated comment on non-blocking streams.

[nherring, 2012-06-14 21:45:21]

LGTM

[jcgregorio_google, 2012-06-15 12:40:21]

On 2012/06/14 21:45:21, nherring wrote:
> LGTM

Committed in
http://code.google.com/p/google-api-python-client/source/detail?r=3ba72a5faf1...