components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java - Issue 1992953004: [Cronet] Make delaying sending request headers explicit in bidirectional stream

Unified Diff: components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java

Issue 1992953004: [Cronet] Make delaying sending request headers explicit in bidirectional stream (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Address Andrei's comment and self review Created 4 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« components/cronet/android/cronet_bidirectional_stream_adapter.h ('K') | « components/cronet/android/cronet_bidirectional_stream_adapter.cc ('k') | components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java » ('j') | components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java

diff --git a/components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java b/components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java

index 188e1766ff57082a5a8052d4e229cf31ecd957dc..c57fcc144708fb39ab8fad04dc197178e66bac9b 100644

--- a/components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java

+++ b/components/cronet/android/java/src/org/chromium/net/CronetBidirectionalStream.java

@@ -41,7 +41,10 @@ class CronetBidirectionalStream extends BidirectionalStream {

private enum State {

/* Initial state, stream not started. */

NOT_STARTED,

- /* Stream started, request headers are being sent. */

+ /*

+ * Stream started, request headers are being sent if mDelayRequestHeadersUntilNextFlush

+ * is not set to true.

+ */

STARTED,

/* Waiting for {@code read()} to be called. */

WAITING_FOR_READ,

@@ -55,8 +58,9 @@ class CronetBidirectionalStream extends BidirectionalStream {

ERROR,

/* Reading and writing are done, and the stream is closed successfully. */

SUCCESS,

- /* Waiting for {@code nativeWritevData()} to be called. */

- WAITING_FOR_WRITE,

+ /* Waiting for {@code nativeSendRequestHeaders()} or {@code nativeWritevData()} to be

+ called. */

+ WAITING_FOR_FLUSH,

/* Writing to the remote, {@code onWritevCompleted()} callback will be called when done. */

WRITING,

/* There is no more data to write and stream is half-closed by the local side. */

@@ -71,6 +75,7 @@ class CronetBidirectionalStream extends BidirectionalStream {

private final String mInitialMethod;

private final String mRequestHeaders[];

private final boolean mDisableAutoFlush;

+ private final boolean mDelayHeadersUntilNextFlush;

* Synchronizes access to mNativeStream, mReadState and mWriteState.

@@ -90,6 +95,10 @@ class CronetBidirectionalStream extends BidirectionalStream {

// Whether an end-of-stream flag is passed in through write().

private boolean mEndOfStreamWritten;

+ @GuardedBy("mNativeStreamLock")

+ // Whether request headers have been flushed.

+ private boolean mRequestHeadersFlushed;

mef 2016/06/01 21:33:21 nit: requestHeadersFlushed => requestHeadersSent t

xunjieli 2016/06/01 22:27:16 Done.

/* Native BidirectionalStream object, owned by CronetBidirectionalStream. */

@GuardedBy("mNativeStreamLock")

private long mNativeStream;

@@ -109,7 +118,7 @@ class CronetBidirectionalStream extends BidirectionalStream {

* / <--- WRITING <--- \

* | |

* \ /

- * NOT_STARTED -> STARTED --> WAITING_FOR_WRITE -> WRITING_DONE -> SUCCESS

+ * NOT_STARTED -> STARTED --> WAITING_FOR_FLUSH -> WRITING_DONE -> SUCCESS

@GuardedBy("mNativeStreamLock")

private State mWriteState = State.NOT_STARTED;

@@ -202,7 +211,7 @@ class CronetBidirectionalStream extends BidirectionalStream {

CronetBidirectionalStream(CronetUrlRequestContext requestContext, String url,

@BidirectionalStream.Builder.StreamPriority int priority, Callback callback,

Executor executor, String httpMethod, List<Map.Entry<String, String>> requestHeaders,

- boolean disableAutoFlush) {

+ boolean disableAutoFlush, boolean delayRequestHeadersUntilNextFlush) {

mRequestContext = requestContext;

mInitialUrl = url;

mInitialPriority = convertStreamPriority(priority);

@@ -211,6 +220,7 @@ class CronetBidirectionalStream extends BidirectionalStream {

mInitialMethod = httpMethod;

mRequestHeaders = stringsFromHeaderList(requestHeaders);

mDisableAutoFlush = disableAutoFlush;

+ mDelayHeadersUntilNextFlush = delayRequestHeadersUntilNextFlush;

mPendingData = new LinkedList<>();

mFlushData = new LinkedList<>();

}

@@ -223,7 +233,8 @@ class CronetBidirectionalStream extends BidirectionalStream {

}

try {

mNativeStream = nativeCreateBidirectionalStream(

- mRequestContext.getUrlRequestContextAdapter(), mDisableAutoFlush);

+ mRequestContext.getUrlRequestContextAdapter(),

+ !mDelayHeadersUntilNextFlush);

mRequestContext.onRequestStarted();

// Non-zero startResult means an argument error.

int startResult = nativeStart(mNativeStream, mInitialUrl, mInitialPriority,

@@ -302,14 +313,25 @@ class CronetBidirectionalStream extends BidirectionalStream {

@SuppressWarnings("GuardedByChecker")

private void flushLocked() {

- if (isDoneLocked()) {

+ if (isDoneLocked()

+ || (mWriteState != State.WAITING_FOR_FLUSH && mWriteState != State.WRITING)) {

return;

}

if (mPendingData.isEmpty() && mFlushData.isEmpty()) {

- // No-op if there is nothing to write.

+ // If there is no pending write when flush() is called, see if

+ // request headers need to be flushed.

+ if (!mRequestHeadersFlushed) {

+ mRequestHeadersFlushed = true;

+ nativeSendRequestHeaders(mNativeStream);

+ if (!doesMethodAllowWriteData(mInitialMethod)) {

+ mWriteState = State.WRITING_DONE;

+ }

return;

}

+ assert !(mPendingData.isEmpty() && mFlushData.isEmpty());

mef 2016/06/01 21:33:20 nit: maybe restate as "assert !mPendingData.isEmpt

xunjieli 2016/06/01 22:27:16 Done.

// Move buffers from mPendingData to the flushing queue.

if (!mPendingData.isEmpty()) {

mFlushData.addAll(mPendingData);

@@ -321,6 +343,7 @@ class CronetBidirectionalStream extends BidirectionalStream {

// called before pushing data to the native stack.

return;

}

+ assert mWriteState == State.WAITING_FOR_FLUSH;

int size = mFlushData.size();

ByteBuffer[] buffers = new ByteBuffer[size];

int[] positions = new int[size];

@@ -332,11 +355,12 @@ class CronetBidirectionalStream extends BidirectionalStream {

limits[i] = buffer.limit();

}

assert mFlushData.isEmpty();

+ assert buffers.length >= 1;

mWriteState = State.WRITING;

if (!nativeWritevData(mNativeStream, buffers, positions, limits, mEndOfStreamWritten)) {

// Still waiting on write. This is just to have consistent

mef 2016/06/01 21:33:21 nit: on write -> for flush

xunjieli 2016/06/01 22:27:16 Done.

// behavior with the other error cases.

- mWriteState = State.WAITING_FOR_WRITE;

+ mWriteState = State.WAITING_FOR_FLUSH;

throw new IllegalArgumentException("Unable to call native writev.");

}

@@ -401,18 +425,19 @@ class CronetBidirectionalStream extends BidirectionalStream {

@SuppressWarnings("unused")

@CalledByNative

- private void onStreamReady() {

+ private void onStreamReady(final boolean requestHeadersFlushed) {

postTaskToExecutor(new Runnable() {

public void run() {

synchronized (mNativeStreamLock) {

if (isDoneLocked()) {

return;

}

- if (doesMethodAllowWriteData(mInitialMethod)) {

- mWriteState = State.WAITING_FOR_WRITE;

- mReadState = State.WAITING_FOR_READ;

- } else {

+ mRequestHeadersFlushed = requestHeadersFlushed;

+ mReadState = State.WAITING_FOR_READ;

+ if (!doesMethodAllowWriteData(mInitialMethod) && mRequestHeadersFlushed) {

mWriteState = State.WRITING_DONE;

+ } else {

+ mWriteState = State.WAITING_FOR_FLUSH;

}

@@ -487,7 +512,7 @@ class CronetBidirectionalStream extends BidirectionalStream {

assert byteBuffers.length == initialPositions.length;

assert byteBuffers.length == initialLimits.length;

synchronized (mNativeStreamLock) {

- mWriteState = State.WAITING_FOR_WRITE;

+ mWriteState = State.WAITING_FOR_FLUSH;

// Flush if there is anything in the flush queue mFlushData.

if (!mFlushData.isEmpty()) {

flushLocked();

@@ -688,13 +713,16 @@ class CronetBidirectionalStream extends BidirectionalStream {

// Native methods are implemented in cronet_bidirectional_stream_adapter.cc.

private native long nativeCreateBidirectionalStream(

- long urlRequestContextAdapter, boolean disableAutoFlush);

+ long urlRequestContextAdapter, boolean sendRequestHeadersAutomatically);

@NativeClassQualifiedName("CronetBidirectionalStreamAdapter")

private native int nativeStart(long nativePtr, String url, int priority, String method,

String[] headers, boolean endOfStream);

@NativeClassQualifiedName("CronetBidirectionalStreamAdapter")

+ private native void nativeSendRequestHeaders(long nativePtr);

+ @NativeClassQualifiedName("CronetBidirectionalStreamAdapter")

private native boolean nativeReadData(

long nativePtr, ByteBuffer byteBuffer, int position, int limit);