Coalesce small buffers in net::BidirectionalStream

components/cronet/android/api/src/org/chromium/net/BidirectionalStream.java

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.net;
 
 import android.support.annotation.IntDef;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
@@ skipping 29 matching lines @@
         private final Executor mExecutor;
         // List of request headers, stored as header field name and value pairs.
         private final ArrayList<Map.Entry<String, String>> mRequestHeaders =
                 new ArrayList<Map.Entry<String, String>>();
 
         // HTTP method for the request. Default to POST.
         private String mHttpMethod = "POST";
         // Priority of the stream. Default is medium.
         @StreamPriority private int mPriority = STREAM_PRIORITY_MEDIUM;
 
+        private boolean mDisableAutoFlush;
+
         /**
          * Creates a builder for {@link BidirectionalStream} objects. All callbacks for
          * generated {@code BidirectionalStream} objects will be invoked on
          * {@code executor}. {@code executor} must not run tasks on the
          * current thread, otherwise the networking operations may block and exceptions
          * may be thrown at shutdown time.
          *
          * @param url the URL for the generated stream
          * @param callback the {@link Callback} object that gets invoked upon different events
          *     occuring
@@ skipping 93 matching lines @@
          * @param priority priority of the stream which should be one of the
          *         {@link #STREAM_PRIORITY_IDLE STREAM_PRIORITY_*} values.
          * @return the builder to facilitate chaining.
          */
         public Builder setPriority(@StreamPriority int priority) {
             mPriority = priority;
             return this;
         }
 
         /**
+         * By default, data is flushed after every {@link #write write()}. This
+         * is to disable auto flush.
+         */
+        public Builder setDisableAutoFlush() {

[kapishnikov, 2016/04/11 23:09:41]

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.

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

Done. Good idea!

+            mDisableAutoFlush = true;
+            return this;
+        }
+
+        /**
          * Creates a {@link BidirectionalStream} using configuration from this
          * {@link Builder}. The returned {@code BidirectionalStream} can then be started
          * by calling {@link BidirectionalStream#start}.
          *
          * @return constructed {@link BidirectionalStream} using configuration from
          *         this {@link Builder}
          */
         public BidirectionalStream build() {
-            return mCronetEngine.createBidirectionalStream(
-                    mUrl, mCallback, mExecutor, mHttpMethod, mRequestHeaders, mPriority);
+            return mCronetEngine.createBidirectionalStream(mUrl, mCallback, mExecutor, mHttpMethod,
+                    mRequestHeaders, mPriority, mDisableAutoFlush);
         }
     }
 
     /**
      * Callback class used to receive callbacks from a {@link BidirectionalStream}.
      */
     public abstract static class Callback {
         /**
-         * Invoked when request headers are sent. Indicates that stream has initiated the request.
-         * Consumer may call {@link BidirectionalStream#write write()} to start writing data.
+         * Invoked when the stream is ready for reading and writing.
+         * Consumer may call {@link BidirectionalStream#read read()} to start

[kapishnikov, 2016/04/11 23:09:41]

Does it mean that the consumer does not need to wait for
onResponseHeadersReceived() before calling read()?

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

Yes, that is allowed.

+         * reading. Consumer may call {@link BidirectionalStream#write write()}
+         * to start writing data.
          *
-         * @param stream the stream on which request headers were sent
+         * @param stream the stream that is ready
          */
-        public abstract void onRequestHeadersSent(BidirectionalStream stream);
+        public abstract void onStreamReady(BidirectionalStream stream);
 
         /**
          * Invoked when initial response headers are received. Headers are available from
          * {@code info.}{@link UrlResponseInfo#getAllHeaders getAllHeaders()}.
          * Consumer must call {@link BidirectionalStream#read read()} to start reading.

[kapishnikov, 2016/04/11 23:09:41]

Javadoc says that the consumer must call read(). What will happen if read() is
called second time before onReadCompleted()?

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

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. 

          * Consumer may call {@link BidirectionalStream#write write()} to start writing or close the
          * stream.
          *
          * @param stream the stream on which response headers were received
          * @param info the response information
          */
         public abstract void onResponseHeadersReceived(
                 BidirectionalStream stream, UrlResponseInfo info);
 
         /**
@@ skipping 106 matching lines @@
 
     /**
      * Starts the stream, all callbacks go to the {@code callback} argument passed to {@link
      * BidirectionalStream.Builder}'s constructor. Should only be called once.
      */
     public abstract void start();
 
     /**
      * Reads data from the stream into the provided buffer.
      * Must only be called at most once in response to each invocation of the
      * {@link Callback#onResponseHeadersReceived onResponseHeadersReceived()} and {@link

[kapishnikov, 2016/04/11 23:09:41]

Related to the previous comments. Does not mention onStreamReady()

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

Done.

      * Callback#onReadCompleted onReadCompleted()} methods of the {@link
      * Callback}. Each call will result in an invocation of one of the
      * {@link Callback Callback}'s {@link Callback#onReadCompleted onReadCompleted()}
      * method if data is read, its {@link Callback#onSucceeded onSucceeded()} method if
      * the stream is closed, or its {@link Callback#onFailed onFailed()} method if
      * there's an error.
      *
      * An attempt to read data into {@code buffer} starting at {@code
      * buffer.position()} is begun. At most {@code buffer.remaining()} bytes are
      * read. {@code buffer.position()} is updated upon invocation of {@link
@@ skipping 29 matching lines @@
      *     {@link Callback#onWriteCompleted onWriteCompleted()}, {@link Callback#onCanceled
      *     onCanceled()}, {@link Callback#onSucceeded onSucceeded()} or {@link Callback#onFailed
      *     onFailed()} are invoked. Can be empty when {@code endOfStream} is {@code true}.
      * @param endOfStream if {@code true}, then {@code buffer} is the last buffer to be written,
      *     and once written, stream is closed from the client side, resulting in half-closed
      *     stream or a fully closed stream if the remote side has already closed.
      */
     public abstract void write(ByteBuffer buffer, boolean endOfStream);
 
     /**
+     * Flushes pending writes. This method should only be invoked when auto
+     * flush is disable through {@link Builder#setDisableAutoFlush}.

[kapishnikov, 2016/04/11 23:09:41]

disable => disabled

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

Done.

+     */
+    public abstract void flush();
+
+    /**
      * Pings remote end-point. {@code callback} methods will be invoked on {@code executor}.
      *
      * @param callback the callback that will be invoked when ping succeeds or fails
      * @param executor the executor on which the callback will be invoked
      */
     // TODO(mef): May be last thing to be implemented on Android.
     public abstract void ping(PingCallback callback, Executor executor);
 
     /**
      * Updates stream flow control window.
@@ skipping 20 matching lines @@
     /**
      * Returns {@code true} if the stream was successfully started and is now
      * done (succeeded, canceled, or failed).
      *
      * @return {@code true} if the stream was successfully started and is now
      *         done (completed, canceled, or failed), otherwise returns {@code false}
      *         to indicate stream is not yet started or is in progress.
      */
     public abstract boolean isDone();
 }