[Cronet] Fix BidirectionalStream.flush()

[Cronet] Fix BidirectionalStream.flush()

If a flush() is delayed because there is a write pending, we will incorrectly
flush data in mPendingQueue as well. This leads to undeterministic behavior
in buffer coalescing. This CL makes delayed flush() to only flush mFlushQueue.

This CL also fixes a bug where end of stream flag is incorrectly set to true
when there is still data in mPendingQueue.

This CL adds a regression test.

BUG=599902
Committed: https://crrev.com/22220beab4feedd951cb23fc2a03019174f18ce0
Cr-Commit-Position: refs/heads/master@{#402163}

[xunjieli, 2016-06-20 16:45:20]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-06-20 16:49:07]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2016-06-20 16:50:11]

xunjieli@chromium.org changed reviewers:
+ kapishnikov@chromium.org, mef@chromium.org

[xunjieli, 2016-06-20 16:50:12]

Misha, Andrei: PTAL. Thanks!

[mef, 2016-06-21 21:12:27]

Sorry, forgot to mail yesterday.

https://codereview.chromium.org/2078353003/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/2078353003/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:87:
private LinkedList<ByteBuffer> mPendingQueue;
Any particular reason for XyzData -> XyzQueue rename?

https://codereview.chromium.org/2078353003/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:554:
buffer, endOfStream && i == byteBuffers.length - 1));
this is a bit hard to understand, maybe it deserves a comment?

[xunjieli, 2016-06-22 14:26:47]

Thanks for the review! PTAL

https://codereview.chromium.org/2078353003/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/2078353003/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:87:
private LinkedList<ByteBuffer> mPendingQueue;
On 2016/06/21 21:12:27, mef wrote:
> Any particular reason for XyzData -> XyzQueue rename?

I felt "Data" is a bit overloaded. I changed it to "Queue" so that it is clearer
that they queues the data/ByteBuffer to be sent.

getFlush[Queue]SizeForTesting() is clear, but getFlush[Data]SizeForTesting()
isn't. The DataSize might be number of bytes, but we really want the size of the
queue.

https://codereview.chromium.org/2078353003/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:554:
buffer, endOfStream && i == byteBuffers.length - 1));
On 2016/06/21 21:12:27, mef wrote:
> this is a bit hard to understand, maybe it deserves a comment?

Done.

[xunjieli, 2016-06-24 17:57:19]

Misha, Andrei: A friendly ping?

[mef, 2016-06-24 19:36:30]

Looks pretty good. 

I'm still not sold on Data -> Queue rename, partially because it means that we
should rename it in iOS counterpart. :)

https://codereview.chromium.org/2078353003/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/2078353003/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:87:
private LinkedList<ByteBuffer> mPendingQueue;
On 2016/06/22 14:26:47, xunjieli wrote:
> On 2016/06/21 21:12:27, mef wrote:
> > Any particular reason for XyzData -> XyzQueue rename?
> 
> I felt "Data" is a bit overloaded. I changed it to "Queue" so that it is
clearer
> that they queues the data/ByteBuffer to be sent.
Yeah, unfortunately Queue is also very generic, and also not very accurate.
More accurate name would be a 'XyzList', right?
> 
> getFlush[Queue]SizeForTesting() is clear, but getFlush[Data]SizeForTesting()
> isn't. The DataSize might be number of bytes, but we really want the size of
the
> queue.
Maybe getFlushDataCountForTesting?

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:554:
// Only set endOfStream flag if this buffer is the last in byteBuffers
nit: End comment with period.

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:287:
// 6 is in pending queue
nit: end comment with period.
Maybe 6 -> "6" here and in other comments to match with data.

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:289:
((CronetBidirectionalStream) stream).getPendingQueueSizeForTesting());
Maybe instead of getPendingQueueSizeForTesting() we should have
getPendingDataForTesting(), which will return (a read-only copy of) actual list
of data that we can check?

[xunjieli, 2016-06-24 20:45:20]

Patchset #3 (id:80001) has been deleted

[xunjieli, 2016-06-24 20:48:23]

Thanks for the review! I've reverted to the old naming scheme and changed the
*ForTesting() methods to return a read-only list. PTAL. Thanks!

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:554:
// Only set endOfStream flag if this buffer is the last in byteBuffers
On 2016/06/24 19:36:30, mef wrote:
> nit: End comment with period.

Done.

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:287:
// 6 is in pending queue
On 2016/06/24 19:36:30, mef wrote:
> nit: end comment with period.
> Maybe 6 -> "6" here and in other comments to match with data.

Done.

https://codereview.chromium.org/2078353003/diff/60001/components/cronet/andro...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:289:
((CronetBidirectionalStream) stream).getPendingQueueSizeForTesting());
On 2016/06/24 19:36:30, mef wrote:
> Maybe instead of getPendingQueueSizeForTesting() we should have
> getPendingDataForTesting(), which will return (a read-only copy of) actual
list
> of data that we can check?

Done. I made a list of read only copies of the byte buffers. I think we will
just be careful in testing so that we are only examine the contents when native
stack isn't trying to modify these byte buffers.

[mef, 2016-06-24 23:30:07]

lgtm

[xunjieli, 2016-06-27 13:22:43]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-06-27 13:22:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 13:59:23]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-06-27 14:01:00]

Description was changed from

==========
[Cronet] Fix BidirectionalStream.flush()

If a flush() is delayed because there is a write pending, we will incorrectly
flush data in mPendingQueue as well. This leads to undeterministic behavior
in buffer coalescing. This CL makes delayed flush() to only flush mFlushQueue.

This CL also fixes a bug where end of stream flag is incorrectly set to true
when there is still data in mPendingQueue.

This CL adds a regression test.

BUG=599902
==========

to

==========
[Cronet] Fix BidirectionalStream.flush()

If a flush() is delayed because there is a write pending, we will incorrectly
flush data in mPendingQueue as well. This leads to undeterministic behavior
in buffer coalescing. This CL makes delayed flush() to only flush mFlushQueue.

This CL also fixes a bug where end of stream flag is incorrectly set to true
when there is still data in mPendingQueue.

This CL adds a regression test.

BUG=599902

Committed: https://crrev.com/22220beab4feedd951cb23fc2a03019174f18ce0
Cr-Commit-Position: refs/heads/master@{#402163}
==========

[commit-bot: I haz the power, 2016-06-27 14:01:01]

Patchset 4 (id:??) landed as
https://crrev.com/22220beab4feedd951cb23fc2a03019174f18ce0
Cr-Commit-Position: refs/heads/master@{#402163}