Coalesce small buffers in net::BidirectionalStream

Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and a flush() method to Cronet's
BidirectionalStream API and buffers writes until flush() is
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler
to bundle small writes into one data packet along with
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small write buffers into a big one.
BidirectionalStreamSpdyImpl doesn't combine header frame
with data frame currently.

BUG=599902
Committed: https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905
Cr-Commit-Position: refs/heads/master@{#389865}

[xunjieli, 2016-04-05 13:49:59]

Description was changed from

==========
TEMP

BUG=
==========

to

==========
Add an explict flush() method to coalesce small buffers in
net::BidirectionalStream

BUG=
==========

[xunjieli, 2016-04-11 19:03:07]

Description was changed from

==========
Add an explict flush() method to coalesce small buffers in
net::BidirectionalStream

BUG=
==========

to

==========
Coalesce small buffers in net::BidirectionalStream

BUG=
==========

[xunjieli, 2016-04-11 19:12:20]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-04-11 19:12:28]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2016-04-11 19:12:36]

Patchset #1 (id:40001) has been deleted

[xunjieli, 2016-04-11 19:12:42]

Patchset #1 (id:60001) has been deleted

[xunjieli, 2016-04-11 19:12:49]

Patchset #1 (id:80001) has been deleted

[xunjieli, 2016-04-11 19:12:57]

Patchset #1 (id:100001) has been deleted

[xunjieli, 2016-04-11 19:13:04]

Patchset #1 (id:120001) has been deleted

[xunjieli, 2016-04-11 19:22:13]

Description was changed from

==========
Coalesce small buffers in net::BidirectionalStream

BUG=
==========

to

==========
Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and flush() method to Cronet's 
BidirectionalStream API and buffers writes until flush() is 
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler 
to bundle small writes into one data packet along with 
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small buffers into a big one. 
BidirectionalStreamSpdyImpl doesn't combine header frame 
with data frame currently.

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

[xunjieli, 2016-04-11 19:25:57]

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

[xunjieli, 2016-04-11 19:25:57]

Hello reviewers! PTAL. This is an early draft, but I'd really appreciate some
feedback.

[xunjieli, 2016-04-11 19:34:49]

Description was changed from

==========
Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and flush() method to Cronet's 
BidirectionalStream API and buffers writes until flush() is 
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler 
to bundle small writes into one data packet along with 
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small buffers into a big one. 
BidirectionalStreamSpdyImpl doesn't combine header frame 
with data frame currently.

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

to

==========
Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and a flush() method to Cronet's 
BidirectionalStream API and buffers writes until flush() is 
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler 
to bundle small writes into one data packet along with 
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small buffers into a big one. 
BidirectionalStreamSpdyImpl doesn't combine header frame 
with data frame currently.

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

[xunjieli, 2016-04-11 19:35:07]

Description was changed from

==========
Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and a flush() method to Cronet's 
BidirectionalStream API and buffers writes until flush() is 
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler 
to bundle small writes into one data packet along with 
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small buffers into a big one. 
BidirectionalStreamSpdyImpl doesn't combine header frame 
with data frame currently.

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

to

==========
Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and a flush() method to Cronet's 
BidirectionalStream API and buffers writes until flush() is 
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler 
to bundle small writes into one data packet along with 
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small write buffers into a big one. 
BidirectionalStreamSpdyImpl doesn't combine header frame 
with data frame currently.

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

[kapishnikov, 2016-04-11 23:09:42]

Comments regarding the Java part.

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:168:
public Builder setDisableAutoFlush() {
I would change the method to accept the boolean parameter and rename it to
disableAutoFlush(Boolean disable).

Here are two reasons.
1. An app can have its own configuration that sets disableAutoFlush flag. It
will be easier for the app to call:
disableAutoFlush(config.disableAutoFlush) rather than to use IF statement.
2. It will make the method independent from the default behavior, e.g. let's say
that for some reason we decided to change the default behaviour to disable
auto-flushing. The method will become no-op and we will need a new method.

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:193:
* Consumer may call {@link BidirectionalStream#read read()} to start
Does it mean that the consumer does not need to wait for
onResponseHeadersReceived() before calling read()?

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:204:
* Consumer must call {@link BidirectionalStream#read read()} to start reading.
Javadoc says that the consumer must call read(). What will happen if read() is
called second time before onReadCompleted()?

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:331:
* {@link Callback#onResponseHeadersReceived onResponseHeadersReceived()} and
{@link
Related to the previous comments. Does not mention onStreamReady()

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:382:
* flush is disable through {@link Builder#setDisableAutoFlush}.
disable => disabled

https://codereview.chromium.org/1856073002/diff/160001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/160001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:85:
// Only relevant if mDisableAutoFlush is true.
Would it be worth to unify the implementation of autoflush and non-autoflush
cases? The only difference I see is that flush() should automatically be called
by write() method when autoflush is enabled and explicitly called by the client
when it is disabled.

The is slight performance impact but it looks negligible.

https://codereview.chromium.org/1856073002/diff/160001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:273:
if (isDoneLocked()) {
Should we throw an IllegalStateException if there is an attempt to write data
when mEndOfStreamWritten is true?

https://codereview.chromium.org/1856073002/diff/160001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:459:
private void onWritevCompleted(final ByteBuffer[] byteBuffers, int[]
initialPositions,
It looks that onWritevCompleted() is called after all byteBuffers have been
written. Would it be better to call onWriteCompleted() for every written buffer;
thus, the client can start filling individual buffers with new data without
waiting for all writes to complete.

[xunjieli, 2016-04-12 18:01:48]

Thanks a lot for the review! PTAL.

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:168:
public Builder setDisableAutoFlush() {
On 2016/04/11 23:09:41, kapishnikov wrote:
> I would change the method to accept the boolean parameter and rename it to
> disableAutoFlush(Boolean disable).
> 
> Here are two reasons.
> 1. An app can have its own configuration that sets disableAutoFlush flag. It
> will be easier for the app to call:
> disableAutoFlush(config.disableAutoFlush) rather than to use IF statement.
> 2. It will make the method independent from the default behavior, e.g. let's
say
> that for some reason we decided to change the default behaviour to disable
> auto-flushing. The method will become no-op and we will need a new method.

Done. Good idea!

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:193:
* Consumer may call {@link BidirectionalStream#read read()} to start
On 2016/04/11 23:09:41, kapishnikov wrote:
> Does it mean that the consumer does not need to wait for
> onResponseHeadersReceived() before calling read()?

Yes, that is allowed.

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:204:
* Consumer must call {@link BidirectionalStream#read read()} to start reading.
On 2016/04/11 23:09:41, kapishnikov wrote:
> Javadoc says that the consumer must call read(). What will happen if read() is
> called second time before onReadCompleted()?

Good catch. I think the Javadoc should read "may" call read(). There is a
runtime check that will occur if there is more than one read() in flight.

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:331:
* {@link Callback#onResponseHeadersReceived onResponseHeadersReceived()} and
{@link
On 2016/04/11 23:09:41, kapishnikov wrote:
> Related to the previous comments. Does not mention onStreamReady()

Done.

https://codereview.chromium.org/1856073002/diff/140001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:382:
* flush is disable through {@link Builder#setDisableAutoFlush}.
On 2016/04/11 23:09:41, kapishnikov wrote:
> disable => disabled

Done.

https://codereview.chromium.org/1856073002/diff/160001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/160001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:85:
// Only relevant if mDisableAutoFlush is true.
On 2016/04/11 23:09:41, kapishnikov wrote:
> Would it be worth to unify the implementation of autoflush and non-autoflush
> cases? The only difference I see is that flush() should automatically be
called
> by write() method when autoflush is enabled and explicitly called by the
client
> when it is disabled.
> 
> The is slight performance impact but it looks negligible.

Done. Although partially. I think we probably should leave the underlying net
implementation having two write versions. I tried to consolidate them, but it
becomes ugly too soon. Let's handle the remaining in a follow-up.

https://codereview.chromium.org/1856073002/diff/160001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:273:
if (isDoneLocked()) {
On 2016/04/11 23:09:42, kapishnikov wrote:
> Should we throw an IllegalStateException if there is an attempt to write data
> when mEndOfStreamWritten is true?

Done.

https://codereview.chromium.org/1856073002/diff/160001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:459:
private void onWritevCompleted(final ByteBuffer[] byteBuffers, int[]
initialPositions,
On 2016/04/11 23:09:42, kapishnikov wrote:
> It looks that onWritevCompleted() is called after all byteBuffers have been
> written. Would it be better to call onWriteCompleted() for every written
buffer;
> thus, the client can start filling individual buffers with new data without
> waiting for all writes to complete.

As discussed, we don't have the signal about each buffer being passed through
the wire.

[kapishnikov, 2016-04-12 19:27:39]

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:267:
if (mEndOfStreamWritten) {
We should move it ahead of
if (isDoneLocked()) {
  return;
}
Writing anything after mEndOfStreamWritten is set should be always illegal.

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:289:
throw new IllegalStateException("flush is called when auto flush is not
disabled.");
Should we allow calling flush even when disableAutoflush is false? It can be
no-op but we should not throw the exception. The reason is that clients may want
to experiment with and without auto-flush enabled. They should not be required
to change the code for that. Also, from the client point of view auto-flushing
means that flush() is called automatically on every write instead of calling it
manually. Calling flush() multiple times should be allowed.

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java
(right):

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:154:
if (flush && !mHandleFlush) {
Related to the previous comment. I think that should be allowed.

[xunjieli, 2016-04-12 19:59:03]

Thanks, Andrei!

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:267:
if (mEndOfStreamWritten) {
On 2016/04/12 19:27:39, kapishnikov wrote:
> We should move it ahead of
> if (isDoneLocked()) {
>   return;
> }
> Writing anything after mEndOfStreamWritten is set should be always illegal.

Done.

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:289:
throw new IllegalStateException("flush is called when auto flush is not
disabled.");
On 2016/04/12 19:27:39, kapishnikov wrote:
> Should we allow calling flush even when disableAutoflush is false? It can be
> no-op but we should not throw the exception. The reason is that clients may
want
> to experiment with and without auto-flush enabled. They should not be required
> to change the code for that. Also, from the client point of view auto-flushing
> means that flush() is called automatically on every write instead of calling
it
> manually. Calling flush() multiple times should be allowed.

Removing the check makes me a bit uneasy, but I think the ability to experiment
without code change is worth it. Done.

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java
(right):

https://codereview.chromium.org/1856073002/diff/180001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/TestBidirectionalStreamCallback.java:154:
if (flush && !mHandleFlush) {
On 2016/04/12 19:27:39, kapishnikov wrote:
> Related to the previous comment. I think that should be allowed.

Done.

[Ryan Hamilton, 2016-04-13 03:42:28]

Looking good!

https://codereview.chromium.org/1856073002/diff/160001/net/quic/bidirectional...
File net/quic/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/1856073002/diff/160001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:135:
input_buffers.push_back(string_data);
Instead of allocating a new std::vector with StringPiece copies of the IOBuffers
and then passing them to a newly created Writev method, how about simply calling
WritevStreamData for each chunk of data to be written?

[xunjieli, 2016-04-13 14:53:15]

Thanks for the review! PTAL.

https://codereview.chromium.org/1856073002/diff/160001/net/quic/bidirectional...
File net/quic/bidirectional_stream_quic_impl.cc (right):

https://codereview.chromium.org/1856073002/diff/160001/net/quic/bidirectional...
net/quic/bidirectional_stream_quic_impl.cc:135:
input_buffers.push_back(string_data);
On 2016/04/13 03:42:28, Ryan Hamilton wrote:
> Instead of allocating a new std::vector with StringPiece copies of the
IOBuffers
> and then passing them to a newly created Writev method, how about simply
calling
> WritevStreamData for each chunk of data to be written?

Done. I got rid of the newly allocated std::vector. But I am not sure if that is
what you meant. Calling WriteStreamData on each chunk is not feasible right?
Because WriteStreamData has a DCHECK to make sure that it is called only when
there is no buffered data. If we get rid of Writev and use WriteStreamData, then
won't we encounter the DCHECK(!HasBufferedData()) ?

[xunjieli, 2016-04-18 17:46:05]

Ryan, Andrei: a friendly ping. Thank you!

[kapishnikov, 2016-04-19 03:22:22]

Helen, sorry for the delay. The CL looks very good. Here are some comments.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:165:
* By default, data is flushed after every {@link #write write()}. This
The first sentence of the method javadoc is used in the "Method Summary" table
to provide the quick summary of all available methods. We should probably start
it with something like "Disables or enables auto flush. By default, data is
flushed after every {@link #write write()}. If the auto flush is disabled, the
client should explicitly call {@link #flush flush()} to flush the data."

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:199:
* @param stream the stream that is ready
"."

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:332:
* Must only be called at most once in response to each invocation of the
The doc says that the method must only be called at most once in response to
each invocation of onStreamReady() and onResponseHeadersReceived(). Is it
allowed to call read() twice (i.e. once for each method) in response to these
methods? If not, should we rephrase it "... at most once in response to either
onStreamReady() or onResponseHeadersReceived()...". Or maybe, we shouldn't
mention onResponseHeadersReceived() at all?

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:64:
WRITING,
Do we need this state? If it is not really used, I think removing it will make
enable and disable auto flush cases more consistent.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:143:
return;
I think we should always call mCallback.onReadCompleted() regardless whether it
was the last read or not. Otherwise the behavior is inconsistent, i.e. sometimes
the last read will trigger onReadCompleted() and sometimes not. Moreover, it is
now inconsistent with OnWriteCompletedRunnable, which always calls
mCallback.onWriteCompleted()

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:175:
maybeSucceedLocked();
The sequence of calls should be mCallback.onWriteCompleted(), then
mCallback.onSucceeded(). Currently, it is possible that mCallback.onSucceeded()
will be called first and then followed by mCallback.onWriteCompleted().

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:280:
mWriteState = State.WRITING;
If we remove WRITING state, we can write:
if (!mDisableAutoFlush) {
    flushLocked();
}
That should be the difference between auto flush enabled/disabled cases.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:310:
throw new IllegalArgumentException("Unable to call native write");
Should we change the state to ERROR here?

[xunjieli, 2016-04-19 19:30:03]

Thanks a lot, Andrei! PTAL.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:165:
* By default, data is flushed after every {@link #write write()}. This
On 2016/04/19 03:22:22, kapishnikov wrote:
> The first sentence of the method javadoc is used in the "Method Summary" table
> to provide the quick summary of all available methods. We should probably
start
> it with something like "Disables or enables auto flush. By default, data is
> flushed after every {@link #write write()}. If the auto flush is disabled, the
> client should explicitly call {@link #flush flush()} to flush the data."

Done.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:199:
* @param stream the stream that is ready
On 2016/04/19 03:22:22, kapishnikov wrote:
> "."

Done.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java:332:
* Must only be called at most once in response to each invocation of the
On 2016/04/19 03:22:22, kapishnikov wrote:
> The doc says that the method must only be called at most once in response to
> each invocation of onStreamReady() and onResponseHeadersReceived(). Is it
> allowed to call read() twice (i.e. once for each method) in response to these
> methods? If not, should we rephrase it "... at most once in response to either
> onStreamReady() or onResponseHeadersReceived()...". Or maybe, we shouldn't
> mention onResponseHeadersReceived() at all?

Done. I agree it can be a bit confusing. I think it is the same case for
write(), since there can only be one write() in flight (if auto flush is on). 
I adjusted the documentation to say that read() can only be called in
onStreamReady() or onResponseHeadersReceived(). Not sure if it's clearer. PTAL.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:64:
WRITING,
On 2016/04/19 03:22:22, kapishnikov wrote:
> Do we need this state? If it is not really used, I think removing it will make
> enable and disable auto flush cases more consistent.

Yes, we need this state to detect that there is only one Write() in fight when
mDisableAutoFlush is false, because the native code can only handle one
in-flight Write() or Writev().

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:143:
return;
On 2016/04/19 03:22:22, kapishnikov wrote:
> I think we should always call mCallback.onReadCompleted() regardless whether
it
> was the last read or not. Otherwise the behavior is inconsistent, i.e.
sometimes
> the last read will trigger onReadCompleted() and sometimes not. Moreover, it
is
> now inconsistent with OnWriteCompletedRunnable, which always calls
> mCallback.onWriteCompleted()

Done. I agree. Although this might break gRPC, depending on how they are calling
read(). But I think it should be fine, since this CL requires API change anyway.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:175:
maybeSucceedLocked();
On 2016/04/19 03:22:22, kapishnikov wrote:
> The sequence of calls should be mCallback.onWriteCompleted(), then
> mCallback.onSucceeded(). Currently, it is possible that
mCallback.onSucceeded()
> will be called first and then followed by mCallback.onWriteCompleted().

No, it isn't possible. onSucceeded is posted asynchronously, so it is run after
onWriteCompleted.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:280:
mWriteState = State.WRITING;
On 2016/04/19 03:22:22, kapishnikov wrote:
> If we remove WRITING state, we can write:
> if (!mDisableAutoFlush) {
>     flushLocked();
> }
> That should be the difference between auto flush enabled/disabled cases.

State.WRITING is needed to detect that there isn't more than one Write() in
flight when mDisableAutoFlush is false. When mDisableAutoFlush() is false, we
flush() automatically after every write(). However, the native code can only
handle one write() or writev() in flight.

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:310:
throw new IllegalArgumentException("Unable to call native write");
On 2016/04/19 03:22:22, kapishnikov wrote:
> Should we change the state to ERROR here? 

Done.

[kapishnikov, 2016-04-19 19:56:39]

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/240001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:175:
maybeSucceedLocked();
On 2016/04/19 19:30:03, xunjieli wrote:
> On 2016/04/19 03:22:22, kapishnikov wrote:
> > The sequence of calls should be mCallback.onWriteCompleted(), then
> > mCallback.onSucceeded(). Currently, it is possible that
> mCallback.onSucceeded()
> > will be called first and then followed by mCallback.onWriteCompleted().
> 
> No, it isn't possible. onSucceeded is posted asynchronously, so it is run
after
> onWriteCompleted.

Do we specify anywhere that the executor should be single-threaded? If it can be
multithreaded then onSucceeded() can be posted on a different thread. The lock
is released before mCallback.onWriteCompleted(...) is called. So, is it possible
that the second thread will execute onSucceeded() faster than the first thread
executes onWriteCompleted().

[xunjieli, 2016-04-20 16:36:39]

Thanks, Andrei, for the incredibly helpful comments and suggestions! I have
uploaded a new patch to allow multiple in-flight Writes. PTAL.

I cleaned up a few APIs to make the read case and write case more in sync. I got
rid of onSucceeded callback, which isn't necessary and adds a few thread hops
per stream. Let me know what you think. Thanks!

[kapishnikov, 2016-04-20 17:43:37]

On 2016/04/20 16:36:39, xunjieli wrote:
> Thanks, Andrei, for the incredibly helpful comments and suggestions! I have
> uploaded a new patch to allow multiple in-flight Writes. PTAL.
> 
> I cleaned up a few APIs to make the read case and write case more in sync. I
got
> rid of onSucceeded callback, which isn't necessary and adds a few thread hops
> per stream. Let me know what you think. Thanks!

Helen, thanks for the changes. I will take a look. I am not sure that we should
get rid of onSuccess(). The method can be quite useful for the client to know
that the the work is done, i.e. that the reads and writes completed. E.g., that
is the place where the client can add logic to release resources. Do you think
it significantly impacts the performance?

[xunjieli, 2016-04-20 19:08:35]

On 2016/04/20 17:43:37, kapishnikov wrote:
> On 2016/04/20 16:36:39, xunjieli wrote:
> > Thanks, Andrei, for the incredibly helpful comments and suggestions! I have
> > uploaded a new patch to allow multiple in-flight Writes. PTAL.
> > 
> > I cleaned up a few APIs to make the read case and write case more in sync. I
> got
> > rid of onSucceeded callback, which isn't necessary and adds a few thread
hops
> > per stream. Let me know what you think. Thanks!
> 
> Helen, thanks for the changes. I will take a look. I am not sure that we
should
> get rid of onSuccess(). The method can be quite useful for the client to know
> that the the work is done, i.e. that the reads and writes completed. E.g.,
that
> is the place where the client can add logic to release resources. Do you think
> it significantly impacts the performance?

Thanks, Andrei! I uploaded a new patch to put back the onSucceeded() callback.

[kapishnikov, 2016-04-20 22:05:43]

Looks great! Here are some extra comments/questions:

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:170:
public OnWriteCompletedRunnable(ByteBuffer buffer, boolean endOfStream) {
Don't need to be public.

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:214:
mPendingData = new LinkedList<ByteBuffer>();
Can be replaced with
mPendingData = new LinkedList<>();
Same for the next line.

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:304:
private void flushLocked() {
Should we call isDoneLocked() here to make sure that the stream is still alive?
E.g. an app can have a logic that when it goes into background it
unconditionally cancels all open connections on the main thread. Meanwhile, some
worker thread may execute flush... or any other method like read() or write().
We should always check that the stream is still alive.

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336:
mWriteState = State.WAITING_FOR_WRITE;
Just to double check. Is it correct to change the state to WAITING_FOR_WRITE?
It does not look that the client can retry by calling flush() again. Is ERROR
more appropriate state?

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:401:
private void onStreamReady() {
Should onStreamReady() set mReadState to WAITING_FOR_READ? Otherwise, read()
will fail but it shouldn't according to the JavaDoc. My assumption is that
onStreamReady() is called before onResponseHeadersReceived(). If this comment is
valid, we need to add a unit test.

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:485:
assert initialPositions.length == initialLimits.length;
This assert is not required because if both variables are equal to
byteBuffers.length, they are mutually equal too.

[xunjieli, 2016-04-21 14:56:25]

Thanks a lot, Andrei! The comments are very useful! You've caught some serious
bugs. Thanks!

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:170:
public OnWriteCompletedRunnable(ByteBuffer buffer, boolean endOfStream) {
On 2016/04/20 22:05:42, kapishnikov wrote:
> Don't need to be public.

Done.

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:214:
mPendingData = new LinkedList<ByteBuffer>();
On 2016/04/20 22:05:42, kapishnikov wrote:
> Can be replaced with
> mPendingData = new LinkedList<>();
> Same for the next line.

Done.

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:304:
private void flushLocked() {
On 2016/04/20 22:05:42, kapishnikov wrote:
> Should we call isDoneLocked() here to make sure that the stream is still
alive?
> E.g. an app can have a logic that when it goes into background it
> unconditionally cancels all open connections on the main thread. Meanwhile,
some
> worker thread may execute flush... or any other method like read() or write().
> We should always check that the stream is still alive.

Done. Good catch! Not checking the adapter is definitely gonna be a problem.

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336:
mWriteState = State.WAITING_FOR_WRITE;
On 2016/04/20 22:05:42, kapishnikov wrote:
> Just to double check. Is it correct to change the state to WAITING_FOR_WRITE?
> It does not look that the client can retry by calling flush() again. Is ERROR
> more appropriate state?

I don't fully understand the comment. I have retained the original behavior and
comment. See original patch line 271 and where nativeReadData is called. 
Looking at the places where IllegalArgumentException has been thrown, we didn't
change the state to ERROR, so I guess we probably should leave the Write case
alone, otherwise we need to change the rest as well?

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:401:
private void onStreamReady() {
On 2016/04/20 22:05:42, kapishnikov wrote:
> Should onStreamReady() set mReadState to WAITING_FOR_READ? Otherwise, read()
> will fail but it shouldn't according to the JavaDoc. My assumption is that
> onStreamReady() is called before onResponseHeadersReceived(). If this comment
is
> valid, we need to add a unit test.

Done. Good point! I added a test as well.

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:485:
assert initialPositions.length == initialLimits.length;
On 2016/04/20 22:05:42, kapishnikov wrote:
> This assert is not required because if both variables are equal to
> byteBuffers.length, they are mutually equal too.

Done.

[kapishnikov, 2016-04-21 15:24:05]

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336:
mWriteState = State.WAITING_FOR_WRITE;
On 2016/04/21 14:56:24, xunjieli wrote:
> On 2016/04/20 22:05:42, kapishnikov wrote:
> > Just to double check. Is it correct to change the state to
WAITING_FOR_WRITE?
> > It does not look that the client can retry by calling flush() again. Is
ERROR
> > more appropriate state?
> 
> I don't fully understand the comment. I have retained the original behavior
and
> comment. See original patch line 271 and where nativeReadData is called. 
> Looking at the places where IllegalArgumentException has been thrown, we
didn't
> change the state to ERROR, so I guess we probably should leave the Write case
> alone, otherwise we need to change the rest as well?

I see. We can address it in another CL. Just two thoughts here:
1) The flush() method does not accept any input arguments. It will be unexpected
for the client to get IllegalArgumentException after calling it.
2) We should assume that the client can do all sort of crazy things. E.g., after
getting the exception, the client can retry flush() call. With the current
implementation, the flush() will succeed but no data will be sent.

[xunjieli, 2016-04-21 16:10:28]

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336:
mWriteState = State.WAITING_FOR_WRITE;
On 2016/04/21 15:24:04, kapishnikov wrote:
> On 2016/04/21 14:56:24, xunjieli wrote:
> > On 2016/04/20 22:05:42, kapishnikov wrote:
> > > Just to double check. Is it correct to change the state to
> WAITING_FOR_WRITE?
> > > It does not look that the client can retry by calling flush() again. Is
> ERROR
> > > more appropriate state?
> > 
> > I don't fully understand the comment. I have retained the original behavior
> and
> > comment. See original patch line 271 and where nativeReadData is called. 
> > Looking at the places where IllegalArgumentException has been thrown, we
> didn't
> > change the state to ERROR, so I guess we probably should leave the Write
case
> > alone, otherwise we need to change the rest as well?
> 
> I see. We can address it in another CL. Just two thoughts here:
> 1) The flush() method does not accept any input arguments. It will be
unexpected
> for the client to get IllegalArgumentException after calling it.
> 2) We should assume that the client can do all sort of crazy things. E.g.,
after
> getting the exception, the client can retry flush() call. With the current
> implementation, the flush() will succeed but no data will be sent.

This is a really good point! Although the exception will only be thrown when a
bad buffer is passed in through Write(), so illegal argument exception sort of
makes sense although it is a bit odd here.

I think (2) is the desired behavior. If the exception is thrown, it means that
that the buffer passed to us is bad, so we can't process it to send any data.
Retrying shouldn't make a difference, right?

[kapishnikov, 2016-04-21 17:06:11]

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336:
mWriteState = State.WAITING_FOR_WRITE;
On 2016/04/21 16:10:27, xunjieli wrote:
> On 2016/04/21 15:24:04, kapishnikov wrote:
> > On 2016/04/21 14:56:24, xunjieli wrote:
> > > On 2016/04/20 22:05:42, kapishnikov wrote:
> > > > Just to double check. Is it correct to change the state to
> > WAITING_FOR_WRITE?
> > > > It does not look that the client can retry by calling flush() again. Is
> > ERROR
> > > > more appropriate state?
> > > 
> > > I don't fully understand the comment. I have retained the original
behavior
> > and
> > > comment. See original patch line 271 and where nativeReadData is called. 
> > > Looking at the places where IllegalArgumentException has been thrown, we
> > didn't
> > > change the state to ERROR, so I guess we probably should leave the Write
> case
> > > alone, otherwise we need to change the rest as well?
> > 
> > I see. We can address it in another CL. Just two thoughts here:
> > 1) The flush() method does not accept any input arguments. It will be
> unexpected
> > for the client to get IllegalArgumentException after calling it.
> > 2) We should assume that the client can do all sort of crazy things. E.g.,
> after
> > getting the exception, the client can retry flush() call. With the current
> > implementation, the flush() will succeed but no data will be sent.
> 
> This is a really good point! Although the exception will only be thrown when a
> bad buffer is passed in through Write(), so illegal argument exception sort of
> makes sense although it is a bit odd here.
> 

Since IllegalArgumentException is unchecked (Runtime) exception, flush() does
not accepts any arguments and we don't mention the exception in the
documentation, in most cases the client will not even think to handle it. This
will probably lead to the application crash. If we want the client to account
for such situations, we should throw a checked exception... or arguably we
should not throw exceptions at all but instead call onError() callback.

> I think (2) is the desired behavior. If the exception is thrown, it means that
> that the buffer passed to us is bad, so we can't process it to send any data.
> Retrying shouldn't make a difference, right?
> 

I agree; however, the current behavior is unclear. One could argue that if
flush() succeeds then all data passed to write() before that should have been
successfully flushed. However, looking at the code that is what is going to
happen. The first call to flush() will empty |mFlushData| and |mPendingData|;
however, the native call may fail. The second call to flush() will check 
|mFlushData| and |mPendingData| length. Since both lengths are now 0, it will
successfully return but no data will be flushed.

Before changing anything, we should decide what do we want the client to do when
the exception is thrown? How the client can recover?

[xunjieli, 2016-04-21 17:16:46]

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336:
mWriteState = State.WAITING_FOR_WRITE;
On 2016/04/21 17:06:11, kapishnikov wrote:
> On 2016/04/21 16:10:27, xunjieli wrote:
> > On 2016/04/21 15:24:04, kapishnikov wrote:
> > > On 2016/04/21 14:56:24, xunjieli wrote:
> > > > On 2016/04/20 22:05:42, kapishnikov wrote:
> > > > > Just to double check. Is it correct to change the state to
> > > WAITING_FOR_WRITE?
> > > > > It does not look that the client can retry by calling flush() again.
Is
> > > ERROR
> > > > > more appropriate state?
> > > > 
> > > > I don't fully understand the comment. I have retained the original
> behavior
> > > and
> > > > comment. See original patch line 271 and where nativeReadData is called.

> > > > Looking at the places where IllegalArgumentException has been thrown, we
> > > didn't
> > > > change the state to ERROR, so I guess we probably should leave the Write
> > case
> > > > alone, otherwise we need to change the rest as well?
> > > 
> > > I see. We can address it in another CL. Just two thoughts here:
> > > 1) The flush() method does not accept any input arguments. It will be
> > unexpected
> > > for the client to get IllegalArgumentException after calling it.
> > > 2) We should assume that the client can do all sort of crazy things. E.g.,
> > after
> > > getting the exception, the client can retry flush() call. With the current
> > > implementation, the flush() will succeed but no data will be sent.
> > 
> > This is a really good point! Although the exception will only be thrown when
a
> > bad buffer is passed in through Write(), so illegal argument exception sort
of
> > makes sense although it is a bit odd here.
> > 
> 
> Since IllegalArgumentException is unchecked (Runtime) exception, flush() does
> not accepts any arguments and we don't mention the exception in the
> documentation, in most cases the client will not even think to handle it. This
> will probably lead to the application crash. If we want the client to account
> for such situations, we should throw a checked exception... or arguably we
> should not throw exceptions at all but instead call onError() callback.
> 
> > I think (2) is the desired behavior. If the exception is thrown, it means
that
> > that the buffer passed to us is bad, so we can't process it to send any
data.
> > Retrying shouldn't make a difference, right?
> > 
> 
> I agree; however, the current behavior is unclear. One could argue that if
> flush() succeeds then all data passed to write() before that should have been
> successfully flushed. However, looking at the code that is what is going to
> happen. The first call to flush() will empty |mFlushData| and |mPendingData|;
> however, the native call may fail. The second call to flush() will check 
> |mFlushData| and |mPendingData| length. Since both lengths are now 0, it will
> successfully return but no data will be flushed.
> 
> Before changing anything, we should decide what do we want the client to do
when
> the exception is thrown? How the client can recover?
> 

Do we want the client to recover from the error? It looks like to me that this
is a programming error which shouldn't be masked. nativeWritevData should only
return false if the buffer is not a direct buffer. There is a check in Write()
that checks whether the buffer is direct, so I don't think this exception will
be even triggered. If the buffer sizes are wrong, that means there is a bug in
Cronet, and we should not mask that and let client recover.

[kapishnikov, 2016-04-21 19:41:25]

Great job! One comment that can be addressed in another CL. LGTM

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336:
mWriteState = State.WAITING_FOR_WRITE;
On 2016/04/21 17:16:46, xunjieli wrote:
> On 2016/04/21 17:06:11, kapishnikov wrote:
> > On 2016/04/21 16:10:27, xunjieli wrote:
> > > On 2016/04/21 15:24:04, kapishnikov wrote:
> > > > On 2016/04/21 14:56:24, xunjieli wrote:
> > > > > On 2016/04/20 22:05:42, kapishnikov wrote:
> > > > > > Just to double check. Is it correct to change the state to
> > > > WAITING_FOR_WRITE?
> > > > > > It does not look that the client can retry by calling flush() again.
> Is
> > > > ERROR
> > > > > > more appropriate state?
> > > > > 
> > > > > I don't fully understand the comment. I have retained the original
> > behavior
> > > > and
> > > > > comment. See original patch line 271 and where nativeReadData is
called.
> 
> > > > > Looking at the places where IllegalArgumentException has been thrown,
we
> > > > didn't
> > > > > change the state to ERROR, so I guess we probably should leave the
Write
> > > case
> > > > > alone, otherwise we need to change the rest as well?
> > > > 
> > > > I see. We can address it in another CL. Just two thoughts here:
> > > > 1) The flush() method does not accept any input arguments. It will be
> > > unexpected
> > > > for the client to get IllegalArgumentException after calling it.
> > > > 2) We should assume that the client can do all sort of crazy things.
E.g.,
> > > after
> > > > getting the exception, the client can retry flush() call. With the
current
> > > > implementation, the flush() will succeed but no data will be sent.
> > > 
> > > This is a really good point! Although the exception will only be thrown
when
> a
> > > bad buffer is passed in through Write(), so illegal argument exception
sort
> of
> > > makes sense although it is a bit odd here.
> > > 
> > 
> > Since IllegalArgumentException is unchecked (Runtime) exception, flush()
does
> > not accepts any arguments and we don't mention the exception in the
> > documentation, in most cases the client will not even think to handle it.
This
> > will probably lead to the application crash. If we want the client to
account
> > for such situations, we should throw a checked exception... or arguably we
> > should not throw exceptions at all but instead call onError() callback.
> > 
> > > I think (2) is the desired behavior. If the exception is thrown, it means
> that
> > > that the buffer passed to us is bad, so we can't process it to send any
> data.
> > > Retrying shouldn't make a difference, right?
> > > 
> > 
> > I agree; however, the current behavior is unclear. One could argue that if
> > flush() succeeds then all data passed to write() before that should have
been
> > successfully flushed. However, looking at the code that is what is going to
> > happen. The first call to flush() will empty |mFlushData| and
|mPendingData|;
> > however, the native call may fail. The second call to flush() will check 
> > |mFlushData| and |mPendingData| length. Since both lengths are now 0, it
will
> > successfully return but no data will be flushed.
> > 
> > Before changing anything, we should decide what do we want the client to do
> when
> > the exception is thrown? How the client can recover?
> > 
> 
> Do we want the client to recover from the error? It looks like to me that this
> is a programming error which shouldn't be masked. nativeWritevData should only
> return false if the buffer is not a direct buffer. There is a check in Write()
> that checks whether the buffer is direct, so I don't think this exception will
> be even triggered. If the buffer sizes are wrong, that means there is a bug in
> Cronet, and we should not mask that and let client recover.
> 

Talked to Helen offline. We should revisit the mWriteState value here and make
it consistent with other places. The problem is that if we allow the client to
recover using the same stream after the IllegalArgumentException, we should
guarantee the stream correct behavior. If we cannot do that, we should change
the state to ERROR.

[xunjieli, 2016-04-21 19:47:52]

Hi Ryan:
Andrei helped me to polish the Java API design. I think the Java side changes
are okay now. Could you take a look at the net/* changes? I am hoping to land
this next week, as I will be out on 4/30 - 5/8. Thank you very much!

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java
(right):

https://codereview.chromium.org/1856073002/diff/340001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java:336:
mWriteState = State.WAITING_FOR_WRITE;
On 2016/04/21 19:41:25, kapishnikov wrote:
> On 2016/04/21 17:16:46, xunjieli wrote:
> > On 2016/04/21 17:06:11, kapishnikov wrote:
> > > On 2016/04/21 16:10:27, xunjieli wrote:
> > > > On 2016/04/21 15:24:04, kapishnikov wrote:
> > > > > On 2016/04/21 14:56:24, xunjieli wrote:
> > > > > > On 2016/04/20 22:05:42, kapishnikov wrote:
> > > > > > > Just to double check. Is it correct to change the state to
> > > > > WAITING_FOR_WRITE?
> > > > > > > It does not look that the client can retry by calling flush()
again.
> > Is
> > > > > ERROR
> > > > > > > more appropriate state?
> > > > > > 
> > > > > > I don't fully understand the comment. I have retained the original
> > > behavior
> > > > > and
> > > > > > comment. See original patch line 271 and where nativeReadData is
> called.
> > 
> > > > > > Looking at the places where IllegalArgumentException has been
thrown,
> we
> > > > > didn't
> > > > > > change the state to ERROR, so I guess we probably should leave the
> Write
> > > > case
> > > > > > alone, otherwise we need to change the rest as well?
> > > > > 
> > > > > I see. We can address it in another CL. Just two thoughts here:
> > > > > 1) The flush() method does not accept any input arguments. It will be
> > > > unexpected
> > > > > for the client to get IllegalArgumentException after calling it.
> > > > > 2) We should assume that the client can do all sort of crazy things.
> E.g.,
> > > > after
> > > > > getting the exception, the client can retry flush() call. With the
> current
> > > > > implementation, the flush() will succeed but no data will be sent.
> > > > 
> > > > This is a really good point! Although the exception will only be thrown
> when
> > a
> > > > bad buffer is passed in through Write(), so illegal argument exception
> sort
> > of
> > > > makes sense although it is a bit odd here.
> > > > 
> > > 
> > > Since IllegalArgumentException is unchecked (Runtime) exception, flush()
> does
> > > not accepts any arguments and we don't mention the exception in the
> > > documentation, in most cases the client will not even think to handle it.
> This
> > > will probably lead to the application crash. If we want the client to
> account
> > > for such situations, we should throw a checked exception... or arguably we
> > > should not throw exceptions at all but instead call onError() callback.
> > > 
> > > > I think (2) is the desired behavior. If the exception is thrown, it
means
> > that
> > > > that the buffer passed to us is bad, so we can't process it to send any
> > data.
> > > > Retrying shouldn't make a difference, right?
> > > > 
> > > 
> > > I agree; however, the current behavior is unclear. One could argue that if
> > > flush() succeeds then all data passed to write() before that should have
> been
> > > successfully flushed. However, looking at the code that is what is going
to
> > > happen. The first call to flush() will empty |mFlushData| and
> |mPendingData|;
> > > however, the native call may fail. The second call to flush() will check 
> > > |mFlushData| and |mPendingData| length. Since both lengths are now 0, it
> will
> > > successfully return but no data will be flushed.
> > > 
> > > Before changing anything, we should decide what do we want the client to
do
> > when
> > > the exception is thrown? How the client can recover?
> > > 
> > 
> > Do we want the client to recover from the error? It looks like to me that
this
> > is a programming error which shouldn't be masked. nativeWritevData should
only
> > return false if the buffer is not a direct buffer. There is a check in
Write()
> > that checks whether the buffer is direct, so I don't think this exception
will
> > be even triggered. If the buffer sizes are wrong, that means there is a bug
in
> > Cronet, and we should not mask that and let client recover.
> > 
> 
> Talked to Helen offline. We should revisit the mWriteState value here and make
> it consistent with other places. The problem is that if we allow the client to
> recover using the same stream after the IllegalArgumentException, we should
> guarantee the stream correct behavior. If we cannot do that, we should change
> the state to ERROR.

Great. Thanks, Andrei. I will file a crbug to address this shortly.

[xunjieli, 2016-04-25 18:04:32]

Patchset #13 (id:380001) has been deleted

[xunjieli, 2016-04-25 18:06:54]

Ryan: Ping. PTAL net/*.  Thanks!

[Ryan Hamilton, 2016-04-26 16:52:16]

QUIC changes LGTM, modulo a suggestion to add a unit test.

https://codereview.chromium.org/1856073002/diff/440001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/1856073002/diff/440001/net/quic/quic_chromium...
net/quic/quic_chromium_client_stream.cc:147: // We should not have data
buffered.
nit: avoid first person in comments. For example:

// Must not be called when data is buffered.

https://codereview.chromium.org/1856073002/diff/440001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_stream.h (right):

https://codereview.chromium.org/1856073002/diff/440001/net/quic/quic_chromium...
net/quic/quic_chromium_client_stream.h:88: const CompletionCallback& callback);
Can you add a test for this method?

[xunjieli, 2016-04-26 18:11:27]

Thanks a lot for the review! I added two tests in 
quic_chromium_client_stream_test.cc (one for the sync case and another for the
async case)

https://codereview.chromium.org/1856073002/diff/440001/net/quic/quic_chromium...
File net/quic/quic_chromium_client_stream.cc (right):

https://codereview.chromium.org/1856073002/diff/440001/net/quic/quic_chromium...
net/quic/quic_chromium_client_stream.cc:147: // We should not have data
buffered.
On 2016/04/26 16:52:16, Ryan Hamilton wrote:
> nit: avoid first person in comments. For example:
> 
> // Must not be called when data is buffered.

Done.

[xunjieli, 2016-04-26 18:11:37]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-04-26 18:11:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
kapishnikov@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1856073002/#ps460001
(title: "Address comments and add two tests in
quic_chromium_client_stream_test.cc")

[commit-bot: I haz the power, 2016-04-26 18:11:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1856073002/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1856073002/460001

[xunjieli, 2016-04-26 18:12:17]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2016-04-26 18:16:21]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-04-26 18:16:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
kapishnikov@chromium.org, rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1856073002/#ps480001
(title: "Fix javadoc")

[commit-bot: I haz the power, 2016-04-26 18:16:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1856073002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1856073002/480001

[commit-bot: I haz the power, 2016-04-26 20:05:44]

Description was changed from

==========
Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and a flush() method to Cronet's 
BidirectionalStream API and buffers writes until flush() is 
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler 
to bundle small writes into one data packet along with 
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small write buffers into a big one. 
BidirectionalStreamSpdyImpl doesn't combine header frame 
with data frame currently.

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

to

==========
Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and a flush() method to Cronet's 
BidirectionalStream API and buffers writes until flush() is 
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler 
to bundle small writes into one data packet along with 
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small write buffers into a big one. 
BidirectionalStreamSpdyImpl doesn't combine header frame 
with data frame currently.

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

[commit-bot: I haz the power, 2016-04-26 20:05:45]

Committed patchset #17 (id:480001)

[commit-bot: I haz the power, 2016-04-26 20:07:09]

Description was changed from

==========
Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and a flush() method to Cronet's 
BidirectionalStream API and buffers writes until flush() is 
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler 
to bundle small writes into one data packet along with 
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small write buffers into a big one. 
BidirectionalStreamSpdyImpl doesn't combine header frame 
with data frame currently.

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

to

==========
Coalesce small buffers in net::BidirectionalStream

This CL adds a flush mode and a flush() method to Cronet's
BidirectionalStream API and buffers writes until flush() is
called. By default, Cronet flushes after every write().

net::BidirectionalStreamQuicImpl uses ScopedPacketBundler
to bundle small writes into one data packet along with
request headers.

net::BidirectionalStreamSpdyImpl will do an extra memcpy
to combine small write buffers into a big one.
BidirectionalStreamSpdyImpl doesn't combine header frame
with data frame currently.

BUG=599902

Committed: https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905
Cr-Commit-Position: refs/heads/master@{#389865}
==========

[commit-bot: I haz the power, 2016-04-26 20:07:10]

Patchset 17 (id:??) landed as
https://crrev.com/07a42ce317dfabf8764d5c3ea2887eef1e467905
Cr-Commit-Position: refs/heads/master@{#389865}