Add Python util to read from a data pipe into a bytearray

Add Python util to read from a data pipe into a bytearray

The implementation of this function is inspired by
//mojo/common/data_pipe_utils.cc. It will be used in an upcoming CL.

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/c0d8adda0cccb7042fc641e5fbaa7a2f6198aaa3

[blundell, 2015-02-25 15:04:29]

blundell@chromium.org changed reviewers:
+ qsr@chromium.org

[blundell, 2015-02-25 15:04:30]

Hi Ben,

I'm looking at bringing up unittests; could you review the production code in
the meantime? Thanks!

[qsr, 2015-02-25 15:22:36]

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
File mojo/python/mojo_utils/data_pipe_utils.py (right):

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:14: BUFFER_SIZE = 1000
Use a power of 2, usual values are 1 to 4k

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:16: output_bytes = bytearray()
Instead of transferring in BUFFER_SIZE chunk, because you keep everything in
memory in the end, you can probably keep your buffer, double its size at each
iteration and fill it by passing views to the ReadData method.

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:28: continue
What happen is you get another result here? In particular, if this is not
RESULT_FAILED_PRECONDITION

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:32: return (mojo_system.RESULT_OK,
output_bytes.decode("utf-8"))
This method being a generic utility method, it should probably return a
bytearray instead.

[qsr, 2015-02-26 09:25:02]

On 2015/02/25 15:22:36, qsr wrote:
>
https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
> File mojo/python/mojo_utils/data_pipe_utils.py (right):
> 
>
https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
> mojo/python/mojo_utils/data_pipe_utils.py:14: BUFFER_SIZE = 1000
> Use a power of 2, usual values are 1 to 4k
> 
>
https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
> mojo/python/mojo_utils/data_pipe_utils.py:16: output_bytes = bytearray()
> Instead of transferring in BUFFER_SIZE chunk, because you keep everything in
> memory in the end, you can probably keep your buffer, double its size at each
> iteration and fill it by passing views to the ReadData method.
> 
>
https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
> mojo/python/mojo_utils/data_pipe_utils.py:28: continue
> What happen is you get another result here? In particular, if this is not
> RESULT_FAILED_PRECONDITION
> 
>
https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
> mojo/python/mojo_utils/data_pipe_utils.py:32: return (mojo_system.RESULT_OK,
> output_bytes.decode("utf-8"))
> This method being a generic utility method, it should probably return a
> bytearray instead.

 BTW,  I have been thinking about this, and I wonder if instead of a blocking
call it would not be better ti return a Promise.

[blundell, 2015-02-26 16:33:53]

Added a method that returns a Promise.

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
File mojo/python/mojo_utils/data_pipe_utils.py (right):

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:14: BUFFER_SIZE = 1000
On 2015/02/25 15:22:35, qsr wrote:
> Use a power of 2, usual values are 1 to 4k

Done.

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:16: output_bytes = bytearray()
On 2015/02/25 15:22:35, qsr wrote:
> Instead of transferring in BUFFER_SIZE chunk, because you keep everything in
> memory in the end, you can probably keep your buffer, double its size at each
> iteration and fill it by passing views to the ReadData method.

Done.

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:28: continue
On 2015/02/25 15:22:35, qsr wrote:
> What happen is you get another result here? In particular, if this is not
> RESULT_FAILED_PRECONDITION

Any result other than OK or FAILED_PRECONDITION is a failure, i.e., should end
up at line 34.

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:32: return (mojo_system.RESULT_OK,
output_bytes.decode("utf-8"))
On 2015/02/25 15:22:35, qsr wrote:
> This method being a generic utility method, it should probably return a
> bytearray instead.

Done.

[qsr, 2015-02-26 16:45:19]

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
File mojo/python/mojo_utils/data_pipe_utils.py (right):

https://codereview.chromium.org/952223002/diff/20001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:28: continue
On 2015/02/26 16:33:53, blundell wrote:
> On 2015/02/25 15:22:35, qsr wrote:
> > What happen is you get another result here? In particular, if this is not
> > RESULT_FAILED_PRECONDITION
> 
> Any result other than OK or FAILED_PRECONDITION is a failure, i.e., should end
> up at line 34.

 Hum, misread the alignment. Ok.

https://codereview.chromium.org/952223002/diff/40001/mojo/python/mojo_utils/d...
File mojo/python/mojo_utils/data_pipe_utils.py (right):

https://codereview.chromium.org/952223002/diff/40001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:17: def GenerationMethod(resolve,
reject):
This is not correct, sorry. You are still blocking. The  CopyFromDataPipe will
synchronously call this method on the current thread.

What you need to do is use data_pipe.AsyncWait and handle your reading whenever
you have data.

[blundell, 2015-03-03 15:58:40]

Patchset #4 (id:60001) has been deleted

[blundell, 2015-03-03 16:04:39]

https://codereview.chromium.org/952223002/diff/40001/mojo/python/mojo_utils/d...
File mojo/python/mojo_utils/data_pipe_utils.py (right):

https://codereview.chromium.org/952223002/diff/40001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:17: def GenerationMethod(resolve,
reject):
On 2015/02/26 16:45:19, qsr wrote:
> This is not correct, sorry. You are still blocking. The  CopyFromDataPipe will
> synchronously call this method on the current thread.
> 
> What you need to do is use data_pipe.AsyncWait and handle your reading
whenever
> you have data.

Done.

[qsr, 2015-03-03 17:10:37]

https://codereview.chromium.org/952223002/diff/80001/mojo/python/mojo_utils/d...
File mojo/python/mojo_utils/data_pipe_utils.py (right):

https://codereview.chromium.org/952223002/diff/80001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:41: return self.original_deadline -
elapsed_time
You probably want a max(0, self.original_deadline - elapsed_time), and so you
probably do not need to special case original_deadline == 0

https://codereview.chromium.org/952223002/diff/80001/mojo/python/tests/data_p...
File mojo/python/tests/data_pipe_utils_unittest.py (right):

https://codereview.chromium.org/952223002/diff/80001/mojo/python/tests/data_p...
mojo/python/tests/data_pipe_utils_unittest.py:50:
self.handles.consumer_handle.Close()
I think you do not need to handle this close yourself, am I wrong?

https://codereview.chromium.org/952223002/diff/80001/mojo/python/tests/data_p...
mojo/python/tests/data_pipe_utils_unittest.py:74: self._runAndCheckError()
You have construction in python unittest when you expect an exception ->
https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises
and co.

https://codereview.chromium.org/952223002/diff/80001/mojo/python/tests/data_p...
mojo/python/tests/data_pipe_utils_unittest.py:85: def
testCloseConsumerWithoutWriting(self):
You should not have this test. This is kind of an undefined behavior.

[blundell, 2015-03-04 10:28:53]

Thanks!

https://codereview.chromium.org/952223002/diff/80001/mojo/python/mojo_utils/d...
File mojo/python/mojo_utils/data_pipe_utils.py (right):

https://codereview.chromium.org/952223002/diff/80001/mojo/python/mojo_utils/d...
mojo/python/mojo_utils/data_pipe_utils.py:41: return self.original_deadline -
elapsed_time
On 2015/03/03 17:10:36, qsr wrote:
> You probably want a max(0, self.original_deadline - elapsed_time), and so you
> probably do not need to special case original_deadline == 0

Done.

https://codereview.chromium.org/952223002/diff/80001/mojo/python/tests/data_p...
File mojo/python/tests/data_pipe_utils_unittest.py (right):

https://codereview.chromium.org/952223002/diff/80001/mojo/python/tests/data_p...
mojo/python/tests/data_pipe_utils_unittest.py:50:
self.handles.consumer_handle.Close()
On 2015/03/03 17:10:37, qsr wrote:
> I think you do not need to handle this close yourself, am I wrong?

Done.

https://codereview.chromium.org/952223002/diff/80001/mojo/python/tests/data_p...
mojo/python/tests/data_pipe_utils_unittest.py:74: self._runAndCheckError()
On 2015/03/03 17:10:36, qsr wrote:
> You have construction in python unittest when you expect an exception ->
> https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises
> and co.

Done.

https://codereview.chromium.org/952223002/diff/80001/mojo/python/tests/data_p...
mojo/python/tests/data_pipe_utils_unittest.py:85: def
testCloseConsumerWithoutWriting(self):
On 2015/03/03 17:10:37, qsr wrote:
> You should not have this test. This is kind of an undefined behavior. 

Done.

[qsr, 2015-03-04 10:35:47]

lgtm

[blundell, 2015-03-04 12:32:03]

Committed patchset #5 (id:100001) manually as
c0d8adda0cccb7042fc641e5fbaa7a2f6198aaa3 (presubmit successful).