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

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

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."

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

Done. 

+         * is to disable auto flush.
+         * @param disableAutoFlush if true, auto flush will be disabled.
+         * @return the builder to facilitate chaining.
+         */
+        public Builder disableAutoFlush(boolean disableAutoFlush) {
+            mDisableAutoFlush = disableAutoFlush;
+            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
+         * 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

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

"."

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

Done.

          */
-        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.
+         * Consumer may call {@link BidirectionalStream#read read()} to start reading.
          * 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 105 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

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

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?

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

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.

+     * {@link Callback#onStreamReady onStreamReady()},
      * {@link Callback#onResponseHeadersReceived onResponseHeadersReceived()} and {@link
      * 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
      * Callback#onReadCompleted onReadCompleted()} to indicate how much data was read.
      *
      * @param buffer the {@link ByteBuffer} to read data into. Must be a
      *     direct ByteBuffer. The embedder must not read or modify buffer's
      *     position, limit, or data between its position and limit until
      *     {@link Callback#onReadCompleted onReadCompleted()}, {@link Callback#onCanceled
      *     onCanceled()}, {@link Callback#onSucceeded onSucceeded()} or {@link Callback#onFailed
      *     onFailed()} are invoked.
      */
     public abstract void read(ByteBuffer buffer);
 
     /**
      * Attempts to write data from the provided buffer into the stream.
      * Must only be called at most once in response to each invocation of the
-     * {@link Callback#onRequestHeadersSent onRequestHeadersSent()} or {@link
+     * {@link Callback#onStreamReady onStreamReady()} or {@link
      * Callback#onWriteCompleted onWriteCompleted()} methods of the {@link Callback}.
      * Each call will result in an asynchronous call to one of the {@link Callback Callback}'s
      * {@link Callback#onWriteCompleted onWriteCompleted()} method if data
      * is sent, its {@link Callback#onSucceeded onSucceeded()} method if stream is closed,
      * or its {@link Callback#onFailed onFailed()} method if there's an error.
      *
      * An attempt to write data from {@code buffer} starting at {@code
      * buffer.position()} is begun. At most {@code buffer.remaining()} bytes are
      * written. {@code buffer.position()} is updated upon invocation of {@link
      * Callback#onWriteCompleted onWriteCompleted()} to indicate how much data was written.
      *
      * @param buffer the {@link ByteBuffer} to write data from. Must be a
      *     direct ByteBuffer. The embedder must not read or modify buffer's
      *     position, limit, or data between its position and limit until
      *     {@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 disabled through {@link Builder#disableAutoFlush}.
+     */
+    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();
 }