From e678e97791dfe8dce8fa1a4620d3904903325ec6 Mon Sep 17 00:00:00 2001 From: mmenke Date: Mon, 20 Apr 2015 11:37:05 -0700 Subject: [PATCH] Cronet: Switch to pull-based API. This lets embedders pause a request by stopping reading from it, and lets them use their own buffer to reduce copies. BUG=412942 Review URL: https://codereview.chromium.org/1075553002 Cr-Commit-Position: refs/heads/master@{#325883} --- .../cronet/android/cronet_url_request_adapter.cc | 79 +++- .../cronet/android/cronet_url_request_adapter.h | 15 +- .../src/org/chromium/net/CronetUrlRequest.java | 246 ++++++----- .../java/src/org/chromium/net/UrlRequest.java | 56 +-- .../src/org/chromium/net/UrlRequestListener.java | 43 +- .../net/urlconnection/CronetHttpURLConnection.java | 16 +- .../net/urlconnection/CronetInputStream.java | 12 +- .../chromium/net/CronetUrlRequestContextTest.java | 86 ++-- .../src/org/chromium/net/CronetUrlRequestTest.java | 460 +++++++++++++++++++-- .../org/chromium/net/TestUrlRequestListener.java | 131 ++++-- .../org/chromium/net/MockUrlRequestJobFactory.java | 2 + 11 files changed, 835 insertions(+), 311 deletions(-) diff --git a/components/cronet/android/cronet_url_request_adapter.cc b/components/cronet/android/cronet_url_request_adapter.cc index 237179984a92..64713bfa4458 100644 --- a/components/cronet/android/cronet_url_request_adapter.cc +++ b/components/cronet/android/cronet_url_request_adapter.cc @@ -4,6 +4,8 @@ #include "cronet_url_request_adapter.h" +#include + #include "base/bind.h" #include "base/location.h" #include "base/logging.h" @@ -23,8 +25,6 @@ using base::android::ConvertUTF8ToJavaString; namespace cronet { -static const int kReadBufferSize = 32768; - // Explicitly register static JNI functions. bool CronetUrlRequestAdapterRegisterJni(JNIEnv* env) { return RegisterNativesImpl(env); @@ -52,6 +52,37 @@ static jlong CreateRequestAdapter(JNIEnv* env, return reinterpret_cast(adapter); } +// IOBuffer subclass for a buffer owned by a Java ByteBuffer. Keeps the +// ByteBuffer alive until destroyed. +class CronetURLRequestAdapter::IOBufferWithByteBuffer : public net::IOBuffer { + public: + // Creates a buffer wrapping the Java ByteBuffer |jbyte_buffer|. |data| points + // to the memory backed by the ByteBuffer, and position is the location to + // start writing. + IOBufferWithByteBuffer( + JNIEnv* env, + jobject jbyte_buffer, + void* data, + int position) + : net::IOBuffer(static_cast(data) + position), + initial_position_(position) { + DCHECK(data); + DCHECK_EQ(env->GetDirectBufferAddress(jbyte_buffer), data); + byte_buffer_.Reset(env, jbyte_buffer); + } + + int initial_position() const { return initial_position_; } + + jobject byte_buffer() const { return byte_buffer_.obj(); } + + private: + ~IOBufferWithByteBuffer() override {} + + base::android::ScopedJavaGlobalRef byte_buffer_; + + const int initial_position_; +}; + CronetURLRequestAdapter::CronetURLRequestAdapter( CronetURLRequestContextAdapter* context, JNIEnv* env, @@ -127,11 +158,27 @@ void CronetURLRequestAdapter::FollowDeferredRedirect(JNIEnv* env, base::Unretained(this))); } -void CronetURLRequestAdapter::ReadData(JNIEnv* env, jobject jcaller) { +jboolean CronetURLRequestAdapter::ReadData( + JNIEnv* env, jobject jcaller, jobject jbyte_buffer, jint jposition, + jint jcapacity) { DCHECK(!context_->IsOnNetworkThread()); + DCHECK_LT(jposition, jcapacity); + + void* data = env->GetDirectBufferAddress(jbyte_buffer); + if (!data) + return JNI_FALSE; + + scoped_refptr read_buffer( + new IOBufferWithByteBuffer(env, jbyte_buffer, data, jposition)); + + int remaining_capacity = jcapacity - jposition; + context_->PostTaskToNetworkThread( FROM_HERE, base::Bind(&CronetURLRequestAdapter::ReadDataOnNetworkThread, - base::Unretained(this))); + base::Unretained(this), + read_buffer, + remaining_capacity)); + return JNI_TRUE; } void CronetURLRequestAdapter::Destroy(JNIEnv* env, jobject jcaller) { @@ -198,7 +245,7 @@ void CronetURLRequestAdapter::OnReceivedRedirect( DCHECK(context_->IsOnNetworkThread()); DCHECK(request->status().is_success()); JNIEnv* env = base::android::AttachCurrentThread(); - cronet::Java_CronetUrlRequest_onRedirect( + cronet::Java_CronetUrlRequest_onReceivedRedirect( env, owner_.obj(), ConvertUTF8ToJavaString(env, redirect_info.new_url.spec()).obj(), redirect_info.status_code); @@ -221,10 +268,12 @@ void CronetURLRequestAdapter::OnReadCompleted(net::URLRequest* request, return; if (bytes_read != 0) { JNIEnv* env = base::android::AttachCurrentThread(); - base::android::ScopedJavaLocalRef java_buffer( - env, env->NewDirectByteBuffer(read_buffer_->data(), bytes_read)); - cronet::Java_CronetUrlRequest_onDataReceived(env, owner_.obj(), - java_buffer.obj()); + cronet::Java_CronetUrlRequest_onReadCompleted( + env, owner_.obj(), read_buffer_->byte_buffer(), bytes_read, + read_buffer_->initial_position()); + // Free the read buffer. This lets the Java ByteBuffer be freed, if the + // embedder releases it, too. + read_buffer_ = nullptr; } else { JNIEnv* env = base::android::AttachCurrentThread(); cronet::Java_CronetUrlRequest_onSucceeded(env, owner_.obj()); @@ -252,13 +301,17 @@ void CronetURLRequestAdapter::FollowDeferredRedirectOnNetworkThread() { url_request_->FollowDeferredRedirect(); } -void CronetURLRequestAdapter::ReadDataOnNetworkThread() { +void CronetURLRequestAdapter::ReadDataOnNetworkThread( + scoped_refptr read_buffer, + int buffer_size) { DCHECK(context_->IsOnNetworkThread()); - if (!read_buffer_.get()) - read_buffer_ = new net::IOBufferWithSize(kReadBufferSize); + DCHECK(read_buffer); + DCHECK(!read_buffer_); + + read_buffer_ = read_buffer; int bytes_read = 0; - url_request_->Read(read_buffer_.get(), read_buffer_->size(), &bytes_read); + url_request_->Read(read_buffer_.get(), buffer_size, &bytes_read); // If IO is pending, wait for the URLRequest to call OnReadCompleted. if (url_request_->status().is_io_pending()) return; diff --git a/components/cronet/android/cronet_url_request_adapter.h b/components/cronet/android/cronet_url_request_adapter.h index 956598dd1152..ad0d87941b87 100644 --- a/components/cronet/android/cronet_url_request_adapter.h +++ b/components/cronet/android/cronet_url_request_adapter.h @@ -14,6 +14,7 @@ #include "base/callback.h" #include "base/location.h" #include "base/macros.h" +#include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" #include "net/base/request_priority.h" #include "net/url_request/url_request.h" @@ -24,7 +25,6 @@ class SingleThreadTaskRunner; } // namespace base namespace net { -class GrowableIOBuffer; class HttpRequestHeaders; class HttpResponseHeaders; class UploadDataStream; @@ -77,7 +77,10 @@ class CronetURLRequestAdapter : public net::URLRequest::Delegate { void FollowDeferredRedirect(JNIEnv* env, jobject jcaller); // Reads more data. - void ReadData(JNIEnv* env, jobject jcaller); + jboolean ReadData(JNIEnv* env, jobject jcaller, + jobject jbyte_buffer, + jint jposition, + jint jcapacity); // Releases all resources for the request and deletes the object itself. void Destroy(JNIEnv* env, jobject jcaller); @@ -118,9 +121,13 @@ class CronetURLRequestAdapter : public net::URLRequest::Delegate { void OnReadCompleted(net::URLRequest* request, int bytes_read) override; private: + class IOBufferWithByteBuffer; + void StartOnNetworkThread(); void FollowDeferredRedirectOnNetworkThread(); - void ReadDataOnNetworkThread(); + void ReadDataOnNetworkThread( + scoped_refptr read_buffer, + int buffer_size); void DestroyOnNetworkThread(); // Checks status of the request_adapter, return false if |is_success()| is @@ -139,7 +146,7 @@ class CronetURLRequestAdapter : public net::URLRequest::Delegate { net::HttpRequestHeaders initial_request_headers_; scoped_ptr upload_; - scoped_refptr read_buffer_; + scoped_refptr read_buffer_; scoped_ptr url_request_; DISALLOW_COPY_AND_ASSIGN(CronetURLRequestAdapter); diff --git a/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java b/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java index eb5770246df0..cd7025757f8c 100644 --- a/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java +++ b/components/cronet/android/java/src/org/chromium/net/CronetUrlRequest.java @@ -32,10 +32,12 @@ import java.util.concurrent.Executor; final class CronetUrlRequest implements UrlRequest { /* Native adapter object, owned by UrlRequest. */ private long mUrlRequestAdapter; + private boolean mStarted = false; private boolean mCanceled = false; - private boolean mInOnDataReceived = false; private boolean mDisableCache = false; + private boolean mWaitingOnRedirect = false; + private boolean mWaitingOnRead = false; /* * Synchronize access to mUrlRequestAdapter, mStarted, mCanceled and @@ -63,17 +65,18 @@ final class CronetUrlRequest implements UrlRequest { private NativeResponseInfo mResponseInfo; /* - * Listener callback is repeatedly called when data is received, so it is - * cached as member variable. + * Listener callback is repeatedly called when each read is completed, so it + * is cached as member variable. */ - private OnDataReceivedRunnable mOnDataReceivedTask; + private OnReadCompletedRunnable mOnReadCompletedTask; static final class HeadersList extends ArrayList> { } - final class OnDataReceivedRunnable implements Runnable { + private final class OnReadCompletedRunnable implements Runnable { ByteBuffer mByteBuffer; + @Override public void run() { if (isCanceled()) { return; @@ -83,32 +86,16 @@ final class CronetUrlRequest implements UrlRequest { if (mUrlRequestAdapter == 0) { return; } - // mByteBuffer is direct buffer backed by native memory, - // and passed to listener, so the request adapter cannot - // be destroyed while listener has access to it. - // Set |mInOnDataReceived| flag during the call to listener - // and destroy adapter immediately after if request was - // cancelled during the call from listener or other thread. - mInOnDataReceived = true; + mWaitingOnRead = true; } - mListener.onDataReceived(CronetUrlRequest.this, - mResponseInfo, mByteBuffer); + // Null out mByteBuffer, out of paranoia. Has to be done before + // mListener call, to avoid any race when there are multiple + // executor threads. + ByteBuffer buffer = mByteBuffer; mByteBuffer = null; - synchronized (mUrlRequestAdapterLock) { - mInOnDataReceived = false; - if (isCanceled()) { - destroyRequestAdapter(); - return; - } - nativeReadData(mUrlRequestAdapter); - } + mListener.onReadCompleted(CronetUrlRequest.this, + mResponseInfo, buffer); } catch (Exception e) { - synchronized (mUrlRequestAdapterLock) { - mInOnDataReceived = false; - if (isCanceled()) { - destroyRequestAdapter(); - } - } onListenerException(e); } } @@ -320,40 +307,71 @@ final class CronetUrlRequest implements UrlRequest { } @Override - public void cancel() { + public void followRedirect() { synchronized (mUrlRequestAdapterLock) { - if (mCanceled || !mStarted) { - return; + if (!mWaitingOnRedirect) { + throw new IllegalStateException("No redirect to follow."); } - mCanceled = true; - // During call into listener OnDataReceived adapter cannot be - // destroyed as it owns the byte buffer. - if (!mInOnDataReceived) { - destroyRequestAdapter(); + mWaitingOnRedirect = false; + + if (isCanceled()) { + return; } + + nativeFollowDeferredRedirect(mUrlRequestAdapter); } } @Override - public boolean isCanceled() { + public void read(ByteBuffer buffer) { synchronized (mUrlRequestAdapterLock) { - return mCanceled; - } - } + if (buffer.position() >= buffer.capacity()) { + throw new IllegalArgumentException( + "ByteBuffer is already full."); + } - @Override - public void pause() { - throw new UnsupportedOperationException("Not implemented yet"); + if (isCanceled()) { + return; + } + + if (!mWaitingOnRead) { + throw new IllegalStateException("Unexpected read attempt."); + } + mWaitingOnRead = false; + + // Indicate buffer has no new data. This is primarily to make it + // clear the buffer has no data in the failure and completion cases. + buffer.limit(buffer.position()); + + if (!nativeReadData(mUrlRequestAdapter, buffer, buffer.position(), + buffer.capacity())) { + // Still waiting on read. This is just to have consistent + // behavior with the other error cases. + mWaitingOnRead = true; + // Since accessing byteBuffer's memory failed, it's presumably + // not a direct ByteBuffer. + throw new IllegalArgumentException( + "byteBuffer must be a direct ByteBuffer."); + } + } } @Override - public boolean isPaused() { - return false; + public void cancel() { + synchronized (mUrlRequestAdapterLock) { + if (mCanceled || !mStarted) { + return; + } + mCanceled = true; + destroyRequestAdapter(); + } } @Override - public void resume() { - throw new UnsupportedOperationException("Not implemented yet"); + public boolean isCanceled() { + synchronized (mUrlRequestAdapterLock) { + return mCanceled; + } } @Override @@ -462,19 +480,32 @@ final class CronetUrlRequest implements UrlRequest { UrlRequestException uploadError = new UrlRequestException("Exception received from UploadDataProvider", e); Log.e(CronetUrlRequestContext.LOG_TAG, "Exception in upload method", e); - // Do not call into listener if request is canceled. - synchronized (mUrlRequestAdapterLock) { - if (isCanceled()) { - return; + failWithException(uploadError); + } + + /** + * Fails the request with an exception. Can be called on any thread. + */ + private void failWithException(final UrlRequestException exception) { + Runnable task = new Runnable() { + public void run() { + synchronized (mUrlRequestAdapterLock) { + if (isCanceled()) { + return; + } + cancel(); + } + try { + mListener.onFailed(CronetUrlRequest.this, + mResponseInfo, + exception); + } catch (Exception e) { + Log.e(CronetUrlRequestContext.LOG_TAG, + "Exception in onError method", e); + } } - cancel(); - } - try { - mListener.onFailed(this, mResponseInfo, uploadError); - } catch (Exception failException) { - Log.e(CronetUrlRequestContext.LOG_TAG, "Exception notifying of failed upload", - failException); - } + }; + postTaskToExecutor(task); } //////////////////////////////////////////////// @@ -493,26 +524,25 @@ final class CronetUrlRequest implements UrlRequest { */ @SuppressWarnings("unused") @CalledByNative - private void onRedirect(final String newLocation, int httpStatusCode) { + private void onReceivedRedirect(final String newLocation, + int httpStatusCode) { final NativeResponseInfo responseInfo = prepareResponseInfoOnNetworkThread(httpStatusCode); + // Have to do this after creating responseInfo. + mUrlChain.add(newLocation); + Runnable task = new Runnable() { public void run() { - if (isCanceled()) { - return; + synchronized (mUrlRequestAdapterLock) { + if (isCanceled()) { + return; + } + mWaitingOnRedirect = true; } + try { - mListener.onRedirect(CronetUrlRequest.this, responseInfo, - newLocation); - synchronized (mUrlRequestAdapterLock) { - if (isCanceled()) { - return; - } - // It is Ok to access mUrlChain not on the network - // thread as the request is waiting to follow redirect. - mUrlChain.add(newLocation); - nativeFollowDeferredRedirect(mUrlRequestAdapter); - } + mListener.onReceivedRedirect(CronetUrlRequest.this, + responseInfo, newLocation); } catch (Exception e) { onListenerException(e); } @@ -531,18 +561,16 @@ final class CronetUrlRequest implements UrlRequest { mResponseInfo = prepareResponseInfoOnNetworkThread(httpStatusCode); Runnable task = new Runnable() { public void run() { - if (isCanceled()) { - return; + synchronized (mUrlRequestAdapterLock) { + if (isCanceled()) { + return; + } + mWaitingOnRead = true; } + try { mListener.onResponseStarted(CronetUrlRequest.this, mResponseInfo); - synchronized (mUrlRequestAdapterLock) { - if (isCanceled()) { - return; - } - nativeReadData(mUrlRequestAdapter); - } } catch (Exception e) { onListenerException(e); } @@ -557,16 +585,29 @@ final class CronetUrlRequest implements UrlRequest { * pauses the request, it remains valid until the request is resumed. * Cancelling the request also invalidates the buffer. * - * @param byteBuffer Received data. + * @param byteBuffer ByteBuffer containing received data, starting at + * initialPosition. Guaranteed to have at least one read byte. Its + * limit has not yet been updated to reflect the bytes read. + * @param bytesRead Number of bytes read. + * @param initialPosition Original position of byteBuffer when passed to + * read(). Used as a minimal check that the buffer hasn't been + * modified while reading from the network. */ @SuppressWarnings("unused") @CalledByNative - private void onDataReceived(final ByteBuffer byteBuffer) { - if (mOnDataReceivedTask == null) { - mOnDataReceivedTask = new OnDataReceivedRunnable(); - } - mOnDataReceivedTask.mByteBuffer = byteBuffer; - postTaskToExecutor(mOnDataReceivedTask); + private void onReadCompleted(final ByteBuffer byteBuffer, int bytesRead, + int initialPosition) { + if (byteBuffer.position() != initialPosition) { + failWithException(new UrlRequestException( + "ByteBuffer modified externally during read", null)); + return; + } + if (mOnReadCompletedTask == null) { + mOnReadCompletedTask = new OnReadCompletedRunnable(); + } + byteBuffer.limit(initialPosition + bytesRead); + mOnReadCompletedTask.mByteBuffer = byteBuffer; + postTaskToExecutor(mOnReadCompletedTask); } /** @@ -619,28 +660,10 @@ final class CronetUrlRequest implements UrlRequest { @SuppressWarnings("unused") @CalledByNative private void onError(final int nativeError, final String errorString) { - Runnable task = new Runnable() { - public void run() { - if (isCanceled()) { - return; - } - // Destroy adapter first, so request context could be shut down - // from the listener. - destroyRequestAdapter(); - try { - UrlRequestException requestError = new UrlRequestException( - "Exception in CronetUrlRequest: " + errorString, - nativeError); - mListener.onFailed(CronetUrlRequest.this, - mResponseInfo, - requestError); - } catch (Exception e) { - Log.e(CronetUrlRequestContext.LOG_TAG, - "Exception in onError method", e); - } - } - }; - postTaskToExecutor(task); + UrlRequestException requestError = new UrlRequestException( + "Exception in CronetUrlRequest: " + errorString, + nativeError); + failWithException(requestError); } /** @@ -674,7 +697,8 @@ final class CronetUrlRequest implements UrlRequest { private native void nativeFollowDeferredRedirect(long nativePtr); @NativeClassQualifiedName("CronetURLRequestAdapter") - private native void nativeReadData(long nativePtr); + private native boolean nativeReadData(long nativePtr, ByteBuffer byteBuffer, + int position, int capacity); @NativeClassQualifiedName("CronetURLRequestAdapter") private native void nativeDestroy(long nativePtr); diff --git a/components/cronet/android/java/src/org/chromium/net/UrlRequest.java b/components/cronet/android/java/src/org/chromium/net/UrlRequest.java index 01f1a0e171f3..dce200d1e5ad 100644 --- a/components/cronet/android/java/src/org/chromium/net/UrlRequest.java +++ b/components/cronet/android/java/src/org/chromium/net/UrlRequest.java @@ -4,6 +4,7 @@ package org.chromium.net; +import java.nio.ByteBuffer; import java.util.concurrent.Executor; /** @@ -66,6 +67,38 @@ public interface UrlRequest { public void start(); /** + * Follows a pending redirect. Must only be called at most once for each + * invocation of the {@link UrlRequestListener#onRedirect onRedirect} method + * of the {@link UrlRequestListener}. + */ + public void followRedirect(); + + /** + * Attempts to read part of the response body into the provided buffer. + * Must only be called at most once in response to each invocation of the + * {@link UrlRequestListener#onResponseStarted onResponseStarted} and {@link + * UrlRequestListener#onReadCompleted onReadCompleted} methods of the {@link + * UrlRequestListener}. Each call will result in an asynchronous call to + * either the {@link UrlRequestListener UrlRequestListener's} + * {@link UrlRequestListener#onReadCompleted onReadCompleted} method if data + * is read, its {@link UrlRequestListener#onSucceeded onSucceeded} method if + * there's no more data to read, or its {@link UrlRequestListener#onFailed + * onFailed} method if there's an error. + * + * @param buffer {@link ByteBuffer} to write response body to. Must be a + * direct ByteBuffer. The embedder must not read or modify buffer's + * position, limit, or data between its position and capacity until the + * request calls back into the {@link URLRequestListener}. If the + * request is cancelled before such a call occurs, it's never safe to + * use the buffer again. + */ + // TODO(mmenke): Should we add some ugliness to allow reclaiming the buffer + // on cancellation? If it's a C++-allocated buffer, then the consumer + // can never safely free it, unless they put off cancelling a request + // until a callback has been invoked. + public void read(ByteBuffer buffer); + + /** * Cancels the request. * * Can be called at any time. If the Executor passed to UrlRequest on @@ -83,29 +116,6 @@ public interface UrlRequest { public boolean isCanceled(); /** - * Can be called at any time, but the request may continue behind the - * scenes, depending on when it's called. None of the listener's methods - * will be called while paused, until and unless the request is resumed. - * (Note: This allows us to have more than one ByteBuffer in flight, - * if we want, as well as allow pausing at any point). - * - * TBD: May need different pause behavior. - */ - public void pause(); - - /** - * Returns True if paused. False if not paused or is cancelled. - * @return - */ - public boolean isPaused(); - - /** - * When resuming, any pending callback to the listener will be called - * asynchronously. - */ - public void resume(); - - /** * Disables cache for the request. If context is not set up to use cache, * this call has no effect. */ diff --git a/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java b/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java index a2a2997a7940..77a9c4db346e 100644 --- a/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java +++ b/components/cronet/android/java/src/org/chromium/net/UrlRequestListener.java @@ -12,22 +12,33 @@ import java.nio.ByteBuffer; */ public interface UrlRequestListener { /** - * Called before following redirects. The redirect will automatically be - * followed, unless the request is paused or cancelled during this - * callback. If the redirect response has a body, it will be ignored. - * This will only be called between start and onResponseStarted. + * Called whenever a redirect is encountered. This will only be called + * between the call to {@link UrlRequest#start} and + * {@link UrlRequestListener#onResponseStarted + * UrlRequestListener.onResponseStarted()}. The body of the redirect + * response, if it has one, will be ignored. + * + * The redirect will not be followed until the URLRequest's + * {@link UrlRequest#followRedirect} method is called, either synchronously + * or asynchronously. * * @param request Request being redirected. * @param info Response information. * @param newLocationUrl Location where request is redirected. */ - public void onRedirect(UrlRequest request, + public void onReceivedRedirect(UrlRequest request, ResponseInfo info, String newLocationUrl); /** - * Called when the final set of headers, after all redirects, - * is received. Can only be called once for each request. + * Called when the final set of headers, after all redirects, is received. + * Will only be called once for each request. + * + * No other UrlRequestListener method will be called for the request, + * including {@link UrlRequestListener#onSucceeded onSucceeded()} and {@link + * UrlRequestListener#onFailed onFailed()}, until {@link UrlRequest#read + * UrlRequest.read()} is called to attempt to start reading the response + * body. * * @param request Request that started to get response. * @param info Response information. @@ -35,16 +46,22 @@ public interface UrlRequestListener { public void onResponseStarted(UrlRequest request, ResponseInfo info); /** - * Called whenever data is received. The ByteBuffer remains - * valid only for the duration of the callback. Or if the callback - * pauses the request, it remains valid until the request is resumed. - * Cancelling the request also invalidates the buffer. + * Called whenever part of the response body has been read. Only part of + * the buffer may be populated, even if the entire response body has not yet + * been consumed. + * + * No other UrlRequestListener method will be called for the request, + * including {@link UrlRequestListener#onSucceeded onSucceeded()} and {@link + * UrlRequestListener#onFailed onFailed()}, until {@link + * UrlRequest#read UrlRequest.read()} is called to attempt to continue + * reading the response body. * * @param request Request that received data. * @param info Response information. - * @param byteBuffer Received data. + * @param byteBuffer The buffer that was passed in to + * {@link UrlRequest#read}, now containing the received data. */ - public void onDataReceived(UrlRequest request, + public void onReadCompleted(UrlRequest request, ResponseInfo info, ByteBuffer byteBuffer); diff --git a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java index d2674f1500af..bed68b04d515 100644 --- a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java +++ b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java @@ -48,7 +48,6 @@ public class CronetHttpURLConnection extends HttpURLConnection { private CronetOutputStream mOutputStream; private ResponseInfo mResponseInfo; private UrlRequestException mException; - private ByteBuffer mResponseByteBuffer; private boolean mOnRedirectCalled = false; private boolean mHasResponse = false; @@ -397,11 +396,12 @@ public class CronetHttpURLConnection extends HttpURLConnection { * Used by {@link CronetInputStream} to get more data from the network * stack. This should only be called after the request has started. Note * that this call might block if there isn't any more data to be read. + * Since byteBuffer is passed to the UrlRequest, it must be a direct + * ByteBuffer. */ - ByteBuffer getMoreData() throws IOException { - mResponseByteBuffer = null; + void getMoreData(ByteBuffer byteBuffer) throws IOException { + mRequest.read(byteBuffer); mMessageLoop.loop(); - return mResponseByteBuffer; } /** @@ -430,17 +430,14 @@ public class CronetHttpURLConnection extends HttpURLConnection { } @Override - public void onDataReceived(UrlRequest request, ResponseInfo info, + public void onReadCompleted(UrlRequest request, ResponseInfo info, ByteBuffer byteBuffer) { mResponseInfo = info; - mResponseByteBuffer = ByteBuffer.allocate(byteBuffer.capacity()); - mResponseByteBuffer.put(byteBuffer); - mResponseByteBuffer.flip(); mMessageLoop.postQuitTask(); } @Override - public void onRedirect(UrlRequest request, ResponseInfo info, + public void onReceivedRedirect(UrlRequest request, ResponseInfo info, String newLocationUrl) { mOnRedirectCalled = true; if (instanceFollowRedirects) { @@ -449,6 +446,7 @@ public class CronetHttpURLConnection extends HttpURLConnection { } catch (MalformedURLException e) { // Ignored. } + mRequest.followRedirect(); } else { mResponseInfo = info; mRequest.cancel(); diff --git a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java index c5fff687942b..76169d810f5b 100644 --- a/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java +++ b/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java @@ -18,6 +18,8 @@ class CronetInputStream extends InputStream { private boolean mResponseDataCompleted; private ByteBuffer mBuffer; + private static final int READ_BUFFER_SIZE = 32 * 1024; + /** * Constructs a CronetInputStream. * @param httpURLConnection the CronetHttpURLConnection that is associated @@ -59,12 +61,20 @@ class CronetInputStream extends InputStream { */ void setResponseDataCompleted() { mResponseDataCompleted = true; + // Nothing else to read, so can free the buffer. + mBuffer = null; } private void getMoreDataIfNeeded() throws IOException { if (!mResponseDataCompleted && !hasUnreadData()) { + // Allocate read buffer if needed. + if (mBuffer == null) { + mBuffer = ByteBuffer.allocateDirect(READ_BUFFER_SIZE); + } + mBuffer.clear(); + // Requests more data from CronetHttpURLConnection. - mBuffer = mHttpURLConnection.getMoreData(); + mHttpURLConnection.getMoreData(mBuffer); } } diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java index def9a9eaeff9..66c32c0523c8 100644 --- a/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java +++ b/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java @@ -13,12 +13,9 @@ import android.test.suitebuilder.annotation.SmallTest; import org.chromium.base.PathUtils; import org.chromium.base.test.util.Feature; -import org.chromium.net.TestUrlRequestListener.FailureType; import org.chromium.net.TestUrlRequestListener.ResponseStep; import java.io.File; -import java.nio.ByteBuffer; -import java.util.Arrays; /** * Test CronetUrlRequestContext. @@ -66,23 +63,6 @@ public class CronetUrlRequestContextTest extends CronetTestBase { */ class ShutdownTestUrlRequestListener extends TestUrlRequestListener { @Override - public void onDataReceived(UrlRequest request, - ResponseInfo info, - ByteBuffer byteBuffer) { - assertTrue(byteBuffer.capacity() != 0); - byte[] receivedDataBefore = new byte[byteBuffer.capacity()]; - byteBuffer.get(receivedDataBefore); - // super will block if necessary. - super.onDataReceived(request, info, byteBuffer); - // |byteBuffer| is still accessible even if 'cancel' was called on - // another thread. - assertTrue(byteBuffer.capacity() != 0); - byte[] receivedDataAfter = new byte[byteBuffer.capacity()]; - byteBuffer.get(receivedDataAfter); - assertTrue(Arrays.equals(receivedDataBefore, receivedDataAfter)); - } - - @Override public void onSucceeded(UrlRequest request, ExtendedResponseInfo info) { super.onSucceeded(request, info); mActivity.mUrlRequestContext.shutdown(); @@ -128,8 +108,7 @@ public class CronetUrlRequestContextTest extends CronetTestBase { TestUrlRequestListener listener = new ShutdownTestUrlRequestListener(); // Block listener when response starts to verify that shutdown fails // if there are active requests. - listener.setFailure(FailureType.BLOCK, - ResponseStep.ON_RESPONSE_STARTED); + listener.setAutoAdvance(false); UrlRequest urlRequest = mActivity.mUrlRequestContext.createRequest( TEST_URL, listener, listener.getExecutor()); urlRequest.start(); @@ -140,7 +119,32 @@ public class CronetUrlRequestContextTest extends CronetTestBase { assertEquals("Cannot shutdown with active requests.", e.getMessage()); } - listener.openBlockedStep(); + + listener.waitForNextStep(); + assertEquals(ResponseStep.ON_RESPONSE_STARTED, listener.mResponseStep); + try { + mActivity.mUrlRequestContext.shutdown(); + fail("Should throw an exception"); + } catch (Exception e) { + assertEquals("Cannot shutdown with active requests.", + e.getMessage()); + } + listener.startNextRead(urlRequest); + + listener.waitForNextStep(); + assertEquals(ResponseStep.ON_READ_COMPLETED, listener.mResponseStep); + try { + mActivity.mUrlRequestContext.shutdown(); + fail("Should throw an exception"); + } catch (Exception e) { + assertEquals("Cannot shutdown with active requests.", + e.getMessage()); + } + + // May not have read all the data, in theory. Just enable auto-advance + // and finish the request. + listener.setAutoAdvance(true); + listener.startNextRead(urlRequest); listener.blockForDone(); } @@ -244,8 +248,7 @@ public class CronetUrlRequestContextTest extends CronetTestBase { TestUrlRequestListener listener = new TestUrlRequestListener(); // Block listener when response starts to verify that shutdown fails // if there are active requests. - listener.setFailure(FailureType.BLOCK, - ResponseStep.ON_RESPONSE_STARTED); + listener.setAutoAdvance(false); UrlRequest urlRequest = mActivity.mUrlRequestContext.createRequest( TEST_URL, listener, listener.getExecutor()); urlRequest.start(); @@ -256,43 +259,14 @@ public class CronetUrlRequestContextTest extends CronetTestBase { assertEquals("Cannot shutdown with active requests.", e.getMessage()); } + listener.waitForNextStep(); + assertEquals(ResponseStep.ON_RESPONSE_STARTED, listener.mResponseStep); urlRequest.cancel(); mActivity.mUrlRequestContext.shutdown(); } @SmallTest @Feature({"Cronet"}) - public void testShutdownAfterCancelDuringOnDataReceived() throws Exception { - mActivity = launchCronetTestApp(); - TestUrlRequestListener listener = new TestUrlRequestListener(); - // Block listener when response starts to verify that shutdown fails - // if there are active requests. - listener.setFailure(FailureType.BLOCK, ResponseStep.ON_DATA_RECEIVED); - UrlRequest urlRequest = mActivity.mUrlRequestContext.createRequest( - MOCK_CRONET_TEST_SUCCESS_URL, listener, listener.getExecutor()); - urlRequest.start(); - try { - mActivity.mUrlRequestContext.shutdown(); - fail("Should throw an exception"); - } catch (Exception e) { - assertEquals("Cannot shutdown with active requests.", - e.getMessage()); - } - // This cancel happens during 'onDataReceived' step, but cancel is - // delayed until listener call returns as it is accessing direct - // data buffer owned by request. - urlRequest.cancel(); - assertTrue(urlRequest.isCanceled()); - Thread.sleep(1000); - // Cancel happens when listener returns. - listener.openBlockedStep(); - Thread.sleep(1000); - - mActivity.mUrlRequestContext.shutdown(); - } - - @SmallTest - @Feature({"Cronet"}) public void testNetLog() throws Exception { Context context = getInstrumentation().getTargetContext(); File directory = new File(PathUtils.getDataDirectory(context)); diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java b/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java index 8134aa685bae..494bad3e5b7d 100644 --- a/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java +++ b/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java @@ -11,6 +11,7 @@ import org.chromium.base.test.util.Feature; import org.chromium.net.TestUrlRequestListener.FailureType; import org.chromium.net.TestUrlRequestListener.ResponseStep; +import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -85,7 +86,7 @@ public class CronetUrlRequestTest extends CronetTestBase { assertEquals(200, listener.mResponseInfo.getHttpStatusCode()); // Default method is 'GET'. assertEquals("GET", listener.mResponseAsString); - assertFalse(listener.mOnRedirectCalled); + assertEquals(0, listener.mRedirectCount); assertEquals(listener.mResponseStep, ResponseStep.ON_SUCCEEDED); checkResponseInfo(listener.mResponseInfo, NativeTestServer.getEchoMethodURL(), 200, "OK"); @@ -93,6 +94,87 @@ public class CronetUrlRequestTest extends CronetTestBase { NativeTestServer.getEchoMethodURL(), 200, "OK"); } + /** + * Tests a redirect by running it step-by-step. Also tests that delaying a + * request works as expected. To make sure there are no unexpected pending + * messages, does a GET between UrlRequestListener callbacks. + */ + @SmallTest + @Feature({"Cronet"}) + public void testRedirectAsync() throws Exception { + // Start the request and wait to see the redirect. + TestUrlRequestListener listener = new TestUrlRequestListener(); + listener.setAutoAdvance(false); + UrlRequest urlRequest = mActivity.mUrlRequestContext.createRequest( + MockUrlRequestJobFactory.REDIRECT_URL, + listener, listener.getExecutor()); + urlRequest.start(); + listener.waitForNextStep(); + + // Check the redirect. + assertEquals(ResponseStep.ON_RECEIVED_REDIRECT, listener.mResponseStep); + assertEquals(1, listener.mRedirectResponseInfoList.size()); + checkResponseInfo(listener.mRedirectResponseInfoList.get(0), + MockUrlRequestJobFactory.REDIRECT_URL, + 302, "Found"); + assertEquals(1, + listener.mRedirectResponseInfoList.get(0).getUrlChain().length); + assertEquals(MockUrlRequestJobFactory.SUCCESS_URL, + listener.mRedirectUrlList.get(0)); + checkResponseInfoHeader(listener.mRedirectResponseInfoList.get(0), + "redirect-header", "header-value"); + + // Wait for an unrelated request to finish. The request should not + // advance until followRedirect is invoked. + testSimpleGet(); + assertEquals(ResponseStep.ON_RECEIVED_REDIRECT, listener.mResponseStep); + assertEquals(1, listener.mRedirectResponseInfoList.size()); + + // Follow the redirect and wait for the next set of headers. + urlRequest.followRedirect(); + listener.waitForNextStep(); + + assertEquals(ResponseStep.ON_RESPONSE_STARTED, listener.mResponseStep); + assertEquals(1, listener.mRedirectResponseInfoList.size()); + assertEquals(200, listener.mResponseInfo.getHttpStatusCode()); + checkResponseInfo(listener.mResponseInfo, + MockUrlRequestJobFactory.SUCCESS_URL, 200, "OK"); + assertEquals(2, listener.mResponseInfo.getUrlChain().length); + assertEquals(MockUrlRequestJobFactory.REDIRECT_URL, + listener.mResponseInfo.getUrlChain()[0]); + assertEquals(MockUrlRequestJobFactory.SUCCESS_URL, + listener.mResponseInfo.getUrlChain()[1]); + + // Wait for an unrelated request to finish. The request should not + // advance until read is invoked. + testSimpleGet(); + assertEquals(ResponseStep.ON_RESPONSE_STARTED, listener.mResponseStep); + + // One read should get all the characters, but best not to depend on + // how much is actually read from the socket at once. + while (!listener.isDone()) { + listener.startNextRead(urlRequest); + listener.waitForNextStep(); + String response = listener.mResponseAsString; + ResponseStep step = listener.mResponseStep; + if (!listener.isDone()) { + assertEquals(ResponseStep.ON_READ_COMPLETED, step); + } + // Should not receive any messages while waiting for another get, + // as the next read has not been started. + testSimpleGet(); + assertEquals(response, listener.mResponseAsString); + assertEquals(step, listener.mResponseStep); + } + assertEquals(ResponseStep.ON_SUCCEEDED, listener.mResponseStep); + assertEquals(MockUrlRequestJobFactory.SUCCESS_BODY, + listener.mResponseAsString); + + // Make sure there are no other pending messages, which would trigger + // asserts in TestURLRequestListener. + testSimpleGet(); + } + @SmallTest @Feature({"Cronet"}) public void testNotFound() throws Exception { @@ -105,7 +187,7 @@ public class CronetUrlRequestTest extends CronetTestBase { "\n\n\nNot found\n" + "

Test page loaded.

\n\n\n", listener.mResponseAsString); - assertFalse(listener.mOnRedirectCalled); + assertEquals(0, listener.mRedirectCount); assertEquals(listener.mResponseStep, ResponseStep.ON_SUCCEEDED); } @@ -310,37 +392,11 @@ public class CronetUrlRequestTest extends CronetTestBase { @SmallTest @Feature({"Cronet"}) - public void testMockRedirect() throws Exception { - TestUrlRequestListener listener = startAndWaitForComplete( - MockUrlRequestJobFactory.REDIRECT_URL); - ResponseInfo mResponseInfo = listener.mResponseInfo; - assertTrue(listener.mOnRedirectCalled); - assertEquals(200, mResponseInfo.getHttpStatusCode()); - assertEquals(1, listener.mRedirectResponseInfoList.size()); - assertEquals(MockUrlRequestJobFactory.SUCCESS_URL, - mResponseInfo.getUrl()); - assertEquals(2, mResponseInfo.getUrlChain().length); - assertEquals(MockUrlRequestJobFactory.REDIRECT_URL, - mResponseInfo.getUrlChain()[0]); - assertEquals(MockUrlRequestJobFactory.SUCCESS_URL, - mResponseInfo.getUrlChain()[1]); - checkResponseInfo(listener.mRedirectResponseInfoList.get(0), - MockUrlRequestJobFactory.REDIRECT_URL, - 302, "Found"); - checkResponseInfoHeader(listener.mRedirectResponseInfoList.get(0), - "redirect-header", "header-value"); - assertTrue(listener.mHttpResponseDataLength != 0); - assertTrue(listener.mOnRedirectCalled); - assertEquals(listener.mResponseStep, ResponseStep.ON_SUCCEEDED); - } - - @SmallTest - @Feature({"Cronet"}) public void testMockMultiRedirect() throws Exception { TestUrlRequestListener listener = startAndWaitForComplete( MockUrlRequestJobFactory.MULTI_REDIRECT_URL); ResponseInfo mResponseInfo = listener.mResponseInfo; - assertTrue(listener.mOnRedirectCalled); + assertEquals(2, listener.mRedirectCount); assertEquals(200, mResponseInfo.getHttpStatusCode()); assertEquals(2, listener.mRedirectResponseInfoList.size()); @@ -380,7 +436,7 @@ public class CronetUrlRequestTest extends CronetTestBase { assertEquals(MockUrlRequestJobFactory.SUCCESS_URL, mResponseInfo.getUrlChain()[2]); assertTrue(listener.mHttpResponseDataLength != 0); - assertTrue(listener.mOnRedirectCalled); + assertEquals(3, listener.mRedirectCount); assertEquals(listener.mResponseStep, ResponseStep.ON_SUCCEEDED); } @@ -391,7 +447,7 @@ public class CronetUrlRequestTest extends CronetTestBase { MockUrlRequestJobFactory.NOTFOUND_URL); assertEquals(404, listener.mResponseInfo.getHttpStatusCode()); assertTrue(listener.mHttpResponseDataLength != 0); - assertFalse(listener.mOnRedirectCalled); + assertEquals(0, listener.mRedirectCount); assertFalse(listener.mOnErrorCalled); assertEquals(listener.mResponseStep, ResponseStep.ON_SUCCEEDED); } @@ -408,7 +464,7 @@ public class CronetUrlRequestTest extends CronetTestBase { assertNull(listener.mResponseInfo); assertNotNull(listener.mError); assertEquals(arbitraryNetError, listener.mError.netError()); - assertFalse(listener.mOnRedirectCalled); + assertEquals(0, listener.mRedirectCount); assertTrue(listener.mOnErrorCalled); assertEquals(listener.mResponseStep, ResponseStep.NOTHING); } @@ -425,7 +481,7 @@ public class CronetUrlRequestTest extends CronetTestBase { assertEquals(200, listener.mResponseInfo.getHttpStatusCode()); assertNotNull(listener.mError); assertEquals(arbitraryNetError, listener.mError.netError()); - assertFalse(listener.mOnRedirectCalled); + assertEquals(0, listener.mRedirectCount); assertTrue(listener.mOnErrorCalled); assertEquals(listener.mResponseStep, ResponseStep.ON_RESPONSE_STARTED); } @@ -442,11 +498,309 @@ public class CronetUrlRequestTest extends CronetTestBase { assertEquals(200, listener.mResponseInfo.getHttpStatusCode()); assertNotNull(listener.mError); assertEquals(arbitraryNetError, listener.mError.netError()); - assertFalse(listener.mOnRedirectCalled); + assertEquals(0, listener.mRedirectCount); assertTrue(listener.mOnErrorCalled); assertEquals(listener.mResponseStep, ResponseStep.ON_RESPONSE_STARTED); } + /** + * Checks that the buffer is updated correctly, when starting at an offset. + */ + @SmallTest + @Feature({"Cronet"}) + public void testSimpleGetBufferUpdates() throws Exception { + TestUrlRequestListener listener = new TestUrlRequestListener(); + listener.setAutoAdvance(false); + // Since the default method is "GET", the expected response body is also + // "GET". + UrlRequest urlRequest = mActivity.mUrlRequestContext.createRequest( + NativeTestServer.getEchoMethodURL(), + listener, listener.getExecutor()); + urlRequest.start(); + listener.waitForNextStep(); + + ByteBuffer readBuffer = ByteBuffer.allocateDirect(5); + readBuffer.put("FOR".getBytes()); + assertEquals(3, readBuffer.position()); + + // Read first two characters of the response ("GE"). It's theoretically + // possible to need one read per character, though in practice, + // shouldn't happen. + while (listener.mResponseAsString.length() < 2) { + assertFalse(listener.isDone()); + urlRequest.read(readBuffer); + listener.waitForNextStep(); + } + + // Make sure the two characters were read. + assertEquals("GE", listener.mResponseAsString); + + // Check the contents of the entire buffer. The first 3 characters + // should not have been changed, and the last two should be the first + // two characters from the response. + assertEquals("FORGE", bufferContentsToString(readBuffer, 0, 5)); + // The limit should now be 5. Position could be either 3 or 4. + assertEquals(5, readBuffer.limit()); + + assertEquals(ResponseStep.ON_READ_COMPLETED, listener.mResponseStep); + + // Start reading from position 3. Since the only remaining character + // from the response is a "T", when the read completes, the buffer + // should contain "FORTE", with a position() of 3 and a limit() of 4. + readBuffer.position(3); + urlRequest.read(readBuffer); + listener.waitForNextStep(); + + // Make sure all three characters of the response have now been read. + assertEquals("GET", listener.mResponseAsString); + + // Check the entire contents of the buffer. Only the third character + // should have been modified. + assertEquals("FORTE", bufferContentsToString(readBuffer, 0, 5)); + + // Make sure position and limit were updated correctly. + assertEquals(3, readBuffer.position()); + assertEquals(4, readBuffer.limit()); + + assertEquals(ResponseStep.ON_READ_COMPLETED, listener.mResponseStep); + + // One more read attempt. The request should complete. + readBuffer.position(1); + readBuffer.limit(5); + urlRequest.read(readBuffer); + listener.waitForNextStep(); + + assertEquals(200, listener.mResponseInfo.getHttpStatusCode()); + assertEquals("GET", listener.mResponseAsString); + checkResponseInfo(listener.mResponseInfo, + NativeTestServer.getEchoMethodURL(), 200, "OK"); + + // Check that buffer contents were not modified. + assertEquals("FORTE", bufferContentsToString(readBuffer, 0, 5)); + + // Buffer limit should be set to original position, and position should + // not have been modified, since nothing was read. + assertEquals(1, readBuffer.position()); + assertEquals(1, readBuffer.limit()); + + assertEquals(ResponseStep.ON_SUCCEEDED, listener.mResponseStep); + + // Make sure there are no other pending messages, which would trigger + // asserts in TestURLRequestListener. + testSimpleGet(); + } + + @SmallTest + @Feature({"Cronet"}) + public void testBadBuffers() throws Exception { + TestUrlRequestListener listener = new TestUrlRequestListener(); + listener.setAutoAdvance(false); + UrlRequest urlRequest = mActivity.mUrlRequestContext.createRequest( + NativeTestServer.getEchoMethodURL(), listener, + listener.getExecutor()); + urlRequest.start(); + listener.waitForNextStep(); + + // Try to read using a full buffer. + try { + ByteBuffer readBuffer = ByteBuffer.allocateDirect(4); + readBuffer.put("full".getBytes()); + urlRequest.read(readBuffer); + fail("Exception not thrown"); + } catch (IllegalArgumentException e) { + assertEquals("ByteBuffer is already full.", + e.getMessage()); + } + + // Try to read using a non-direct buffer. + try { + ByteBuffer readBuffer = ByteBuffer.allocate(5); + urlRequest.read(readBuffer); + fail("Exception not thrown"); + } catch (IllegalArgumentException e) { + assertEquals("byteBuffer must be a direct ByteBuffer.", + e.getMessage()); + } + + // Finish the request with a direct ByteBuffer. + listener.setAutoAdvance(true); + ByteBuffer readBuffer = ByteBuffer.allocateDirect(5); + urlRequest.read(readBuffer); + listener.blockForDone(); + assertEquals(200, listener.mResponseInfo.getHttpStatusCode()); + assertEquals("GET", listener.mResponseAsString); + } + + @SmallTest + @Feature({"Cronet"}) + public void testUnexpectedReads() throws Exception { + final TestUrlRequestListener listener = new TestUrlRequestListener(); + listener.setAutoAdvance(false); + final UrlRequest urlRequest = + mActivity.mUrlRequestContext.createRequest( + MockUrlRequestJobFactory.REDIRECT_URL, listener, + listener.getExecutor()); + + // Try to read before starting request. + try { + listener.startNextRead(urlRequest); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("Unexpected read attempt.", + e.getMessage()); + } + + // Verify reading right after start throws an assertion. Both must be + // invoked on the Executor thread, to prevent receiving data until after + // startNextRead has been invoked. + Runnable startAndRead = new Runnable() { + @Override + public void run() { + urlRequest.start(); + try { + listener.startNextRead(urlRequest); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("Unexpected read attempt.", + e.getMessage()); + } + } + }; + listener.getExecutor().execute(startAndRead); + listener.waitForNextStep(); + + assertEquals(listener.mResponseStep, ResponseStep.ON_RECEIVED_REDIRECT); + // Try to read after the redirect. + try { + listener.startNextRead(urlRequest); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("Unexpected read attempt.", + e.getMessage()); + } + urlRequest.followRedirect(); + listener.waitForNextStep(); + + assertEquals(listener.mResponseStep, ResponseStep.ON_RESPONSE_STARTED); + assertEquals(200, listener.mResponseInfo.getHttpStatusCode()); + + while (!listener.isDone()) { + Runnable readTwice = new Runnable() { + @Override + public void run() { + listener.startNextRead(urlRequest); + // Try to read again before the last read completes. + try { + listener.startNextRead(urlRequest); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("Unexpected read attempt.", + e.getMessage()); + } + } + }; + listener.getExecutor().execute(readTwice); + listener.waitForNextStep(); + } + + assertEquals(listener.mResponseStep, ResponseStep.ON_SUCCEEDED); + assertEquals(MockUrlRequestJobFactory.SUCCESS_BODY, + listener.mResponseAsString); + + // Try to read after request is complete. + try { + listener.startNextRead(urlRequest); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("Unexpected read attempt.", + e.getMessage()); + } + } + + @SmallTest + @Feature({"Cronet"}) + public void testUnexpectedFollowRedirects() throws Exception { + final TestUrlRequestListener listener = new TestUrlRequestListener(); + listener.setAutoAdvance(false); + final UrlRequest urlRequest = + mActivity.mUrlRequestContext.createRequest( + MockUrlRequestJobFactory.REDIRECT_URL, listener, + listener.getExecutor()); + + // Try to follow a redirect before starting the request. + try { + urlRequest.followRedirect(); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("No redirect to follow.", + e.getMessage()); + } + + // Try to follow a redirect just after starting the request. Has to be + // done on the executor thread to avoid a race. + Runnable startAndRead = new Runnable() { + @Override + public void run() { + urlRequest.start(); + try { + urlRequest.followRedirect(); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("No redirect to follow.", + e.getMessage()); + } + } + }; + listener.getExecutor().execute(startAndRead); + listener.waitForNextStep(); + + assertEquals(listener.mResponseStep, ResponseStep.ON_RECEIVED_REDIRECT); + // Try to follow the redirect twice. Second attempt should fail. + Runnable followRedirectTwice = new Runnable() { + @Override + public void run() { + urlRequest.followRedirect(); + try { + urlRequest.followRedirect(); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("No redirect to follow.", + e.getMessage()); + } + } + }; + listener.getExecutor().execute(followRedirectTwice); + listener.waitForNextStep(); + + assertEquals(listener.mResponseStep, ResponseStep.ON_RESPONSE_STARTED); + assertEquals(200, listener.mResponseInfo.getHttpStatusCode()); + + while (!listener.isDone()) { + try { + urlRequest.followRedirect(); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("No redirect to follow.", + e.getMessage()); + } + listener.startNextRead(urlRequest); + listener.waitForNextStep(); + } + + assertEquals(listener.mResponseStep, ResponseStep.ON_SUCCEEDED); + assertEquals(MockUrlRequestJobFactory.SUCCESS_BODY, + listener.mResponseAsString); + + // Try to follow redirect after request is complete. + try { + urlRequest.followRedirect(); + fail("Exception not thrown"); + } catch (IllegalStateException e) { + assertEquals("No redirect to follow.", + e.getMessage()); + } + } + @SmallTest @Feature({"Cronet"}) public void testUploadSetDataProvider() throws Exception { @@ -867,7 +1221,7 @@ public class CronetUrlRequestTest extends CronetTestBase { TestUrlRequestListener listener = new TestUrlRequestListener(); UrlRequest urlRequest = mActivity.mUrlRequestContext.createRequest( NativeTestServer.getEchoBodyURL(), listener, listener.getExecutor()); - // Shut down the test server, so connecting to it fails. Note that + // Shut down the test server, so connecting to it fails. Note that // calling shutdown again during teardown is safe. NativeTestServer.shutdownNativeTestServer(); @@ -893,7 +1247,7 @@ public class CronetUrlRequestTest extends CronetTestBase { listener, listener.getExecutor()); urlRequest.start(); listener.blockForDone(); - assertTrue(listener.mOnRedirectCalled); + assertEquals(1, listener.mRedirectCount); assertEquals(listener.mResponseStep, failureStep); assertTrue(urlRequest.isCanceled()); assertEquals(expectResponseInfo, listener.mResponseInfo != null); @@ -904,24 +1258,31 @@ public class CronetUrlRequestTest extends CronetTestBase { @SmallTest @Feature({"Cronet"}) public void testFailures() throws Exception { - throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_REDIRECT, + throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_RECEIVED_REDIRECT, + false, false); + throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_RECEIVED_REDIRECT, false, false); - throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_REDIRECT, + throwOrCancel(FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, ResponseStep.ON_RECEIVED_REDIRECT, false, false); - throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_REDIRECT, + throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_RECEIVED_REDIRECT, false, true); throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_RESPONSE_STARTED, true, false); throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_RESPONSE_STARTED, true, false); + throwOrCancel(FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, ResponseStep.ON_RESPONSE_STARTED, + true, false); throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_RESPONSE_STARTED, true, true); - throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_DATA_RECEIVED, + + throwOrCancel(FailureType.CANCEL_SYNC, ResponseStep.ON_READ_COMPLETED, + true, false); + throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_READ_COMPLETED, true, false); - throwOrCancel(FailureType.CANCEL_ASYNC, ResponseStep.ON_DATA_RECEIVED, + throwOrCancel(FailureType.CANCEL_ASYNC_WITHOUT_PAUSE, ResponseStep.ON_READ_COMPLETED, true, false); - throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_DATA_RECEIVED, + throwOrCancel(FailureType.THROW_SYNC, ResponseStep.ON_READ_COMPLETED, true, true); } @@ -935,11 +1296,24 @@ public class CronetUrlRequestTest extends CronetTestBase { listener, listener.getExecutor()); urlRequest.start(); listener.blockForDone(); - assertTrue(listener.mOnRedirectCalled); + assertEquals(1, listener.mRedirectCount); assertEquals(listener.mResponseStep, ResponseStep.ON_SUCCEEDED); assertFalse(urlRequest.isCanceled()); assertNotNull(listener.mResponseInfo); assertNull(listener.mError); assertFalse(listener.mOnErrorCalled); } + + // Returns the contents of byteBuffer, from its position() to its limit(), + // as a String. Does not modify byteBuffer's position(). + private String bufferContentsToString(ByteBuffer byteBuffer, int start, + int end) { + // Use a duplicate to avoid modifying byteBuffer. + ByteBuffer duplicate = byteBuffer.duplicate(); + duplicate.position(start); + duplicate.limit(end); + byte[] contents = new byte[duplicate.remaining()]; + duplicate.get(contents); + return new String(contents); + } } diff --git a/components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestListener.java b/components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestListener.java index 37b8b5f122ce..cd4363e28320 100644 --- a/components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestListener.java +++ b/components/cronet/android/test/javatests/src/org/chromium/net/TestUrlRequestListener.java @@ -7,6 +7,7 @@ package org.chromium.net; import android.os.ConditionVariable; import static junit.framework.Assert.assertEquals; +import static junit.framework.Assert.assertNull; import static junit.framework.Assert.assertTrue; import java.nio.ByteBuffer; @@ -24,31 +25,40 @@ import java.util.concurrent.ThreadFactory; class TestUrlRequestListener implements UrlRequestListener { public ArrayList mRedirectResponseInfoList = new ArrayList(); + public ArrayList mRedirectUrlList = new ArrayList(); public ResponseInfo mResponseInfo; public ExtendedResponseInfo mExtendedResponseInfo; public UrlRequestException mError; public ResponseStep mResponseStep = ResponseStep.NOTHING; - public boolean mOnRedirectCalled = false; + public int mRedirectCount = 0; public boolean mOnErrorCalled = false; public int mHttpResponseDataLength = 0; public byte[] mLastDataReceivedAsBytes; public String mResponseAsString = ""; + private static final int READ_BUFFER_SIZE = 32 * 1024; + + // When false, the consumer is responsible for all calls into the request + // that advance it. + private boolean mAutoAdvance = true; + // Conditionally fail on certain steps. private FailureType mFailureType = FailureType.NONE; private ResponseStep mFailureStep = ResponseStep.NOTHING; // Signals when request is done either successfully or not. - private ConditionVariable mDone = new ConditionVariable(); - private ConditionVariable mStepBlock = new ConditionVariable(true); + private final ConditionVariable mDone = new ConditionVariable(); + + // Signaled on each step when mAutoAdvance is false. + private final ConditionVariable mStepBlock = new ConditionVariable(); // Executor for Cronet callbacks. - ExecutorService mExecutor = Executors.newSingleThreadExecutor( + private final ExecutorService mExecutor = Executors.newSingleThreadExecutor( new ExecutorThreadFactory()); - Thread mExecutorThread; + private Thread mExecutorThread; private class ExecutorThreadFactory implements ThreadFactory { public Thread newThread(Runnable r) { @@ -59,37 +69,38 @@ class TestUrlRequestListener implements UrlRequestListener { public enum ResponseStep { NOTHING, - ON_REDIRECT, + ON_RECEIVED_REDIRECT, ON_RESPONSE_STARTED, - ON_DATA_RECEIVED, + ON_READ_COMPLETED, ON_SUCCEEDED }; public enum FailureType { NONE, - BLOCK, CANCEL_SYNC, CANCEL_ASYNC, + // Same as above, but continues to advance the request after posting + // the cancellation task. + CANCEL_ASYNC_WITHOUT_PAUSE, THROW_SYNC }; - public TestUrlRequestListener() { + public void setAutoAdvance(boolean autoAdvance) { + mAutoAdvance = autoAdvance; } public void setFailure(FailureType failureType, ResponseStep failureStep) { mFailureStep = failureStep; mFailureType = failureType; - if (failureType == FailureType.BLOCK) { - mStepBlock.close(); - } } public void blockForDone() { mDone.block(); } - public void openBlockedStep() { - mStepBlock.open(); + public void waitForNextStep() { + mStepBlock.block(); + mStepBlock.close(); } public Executor getExecutor() { @@ -97,53 +108,76 @@ class TestUrlRequestListener implements UrlRequestListener { } @Override - public void onRedirect(UrlRequest request, + public void onReceivedRedirect(UrlRequest request, ResponseInfo info, String newLocationUrl) { assertEquals(mExecutorThread, Thread.currentThread()); assertTrue(mResponseStep == ResponseStep.NOTHING - || mResponseStep == ResponseStep.ON_REDIRECT); + || mResponseStep == ResponseStep.ON_RECEIVED_REDIRECT); + assertNull(mError); + + mResponseStep = ResponseStep.ON_RECEIVED_REDIRECT; + mRedirectUrlList.add(newLocationUrl); mRedirectResponseInfoList.add(info); - mResponseStep = ResponseStep.ON_REDIRECT; - mOnRedirectCalled = true; - maybeThrowOrCancel(request); + ++mRedirectCount; + if (maybeThrowCancelOrPause(request)) { + return; + } + request.followRedirect(); } @Override public void onResponseStarted(UrlRequest request, ResponseInfo info) { assertEquals(mExecutorThread, Thread.currentThread()); assertTrue(mResponseStep == ResponseStep.NOTHING - || mResponseStep == ResponseStep.ON_REDIRECT); + || mResponseStep == ResponseStep.ON_RECEIVED_REDIRECT); + assertNull(mError); + mResponseStep = ResponseStep.ON_RESPONSE_STARTED; mResponseInfo = info; - maybeThrowOrCancel(request); + if (maybeThrowCancelOrPause(request)) { + return; + } + startNextRead(request); } @Override - public void onDataReceived(UrlRequest request, + public void onReadCompleted(UrlRequest request, ResponseInfo info, ByteBuffer byteBuffer) { assertEquals(mExecutorThread, Thread.currentThread()); + assertTrue(byteBuffer.remaining() > 0); assertTrue(mResponseStep == ResponseStep.ON_RESPONSE_STARTED - || mResponseStep == ResponseStep.ON_DATA_RECEIVED); - mResponseStep = ResponseStep.ON_DATA_RECEIVED; + || mResponseStep == ResponseStep.ON_READ_COMPLETED); + assertNull(mError); - mHttpResponseDataLength += byteBuffer.capacity(); - mLastDataReceivedAsBytes = new byte[byteBuffer.capacity()]; - byteBuffer.get(mLastDataReceivedAsBytes); + mResponseStep = ResponseStep.ON_READ_COMPLETED; + + // Make a slice of ByteBuffer, so can read from it without affecting + // position, which allows tests to check the state of the buffer. + ByteBuffer slice = byteBuffer.slice(); + mHttpResponseDataLength += slice.remaining(); + mLastDataReceivedAsBytes = new byte[slice.remaining()]; + slice.get(mLastDataReceivedAsBytes); mResponseAsString += new String(mLastDataReceivedAsBytes); - maybeThrowOrCancel(request); + + if (maybeThrowCancelOrPause(request)) { + return; + } + startNextRead(request); } @Override public void onSucceeded(UrlRequest request, ExtendedResponseInfo info) { assertEquals(mExecutorThread, Thread.currentThread()); assertTrue(mResponseStep == ResponseStep.ON_RESPONSE_STARTED - || mResponseStep == ResponseStep.ON_DATA_RECEIVED); + || mResponseStep == ResponseStep.ON_READ_COMPLETED); + assertTrue(mError == null); + mResponseStep = ResponseStep.ON_SUCCEEDED; mExtendedResponseInfo = info; openDone(); - maybeThrowOrCancel(request); + maybeThrowCancelOrPause(request); } @Override @@ -151,23 +185,42 @@ class TestUrlRequestListener implements UrlRequestListener { ResponseInfo info, UrlRequestException error) { assertEquals(mExecutorThread, Thread.currentThread()); + // Shouldn't happen after success. + assertTrue(mResponseStep != ResponseStep.ON_SUCCEEDED); + mOnErrorCalled = true; mError = error; openDone(); - maybeThrowOrCancel(request); + maybeThrowCancelOrPause(request); + } + + public void startNextRead(UrlRequest request) { + request.read(ByteBuffer.allocateDirect(READ_BUFFER_SIZE)); + } + + public boolean isDone() { + // It's not mentioned by the Android docs, but block(0) seems to block + // indefinitely, so have to block for one millisecond to get state + // without blocking. + return mDone.block(1); } protected void openDone() { mDone.open(); } - private void maybeThrowOrCancel(final UrlRequest request) { - if (mResponseStep != mFailureStep) { - return; - } - if (mFailureType == FailureType.NONE) { - return; + /** + * Returns false if the listener should continue to advance the request. + */ + private boolean maybeThrowCancelOrPause(final UrlRequest request) { + if (mResponseStep != mFailureStep || mFailureType == FailureType.NONE) { + if (!mAutoAdvance) { + mStepBlock.open(); + return true; + } + return false; } + if (mFailureType == FailureType.THROW_SYNC) { throw new IllegalStateException("Listener Exception."); } @@ -177,11 +230,13 @@ class TestUrlRequestListener implements UrlRequestListener { openDone(); } }; - if (mFailureType == FailureType.CANCEL_ASYNC) { + if (mFailureType == FailureType.CANCEL_ASYNC + || mFailureType == FailureType.CANCEL_ASYNC_WITHOUT_PAUSE) { mExecutor.execute(task); } else { task.run(); } + return mFailureType != FailureType.CANCEL_ASYNC_WITHOUT_PAUSE; } } diff --git a/components/cronet/android/test/src/org/chromium/net/MockUrlRequestJobFactory.java b/components/cronet/android/test/src/org/chromium/net/MockUrlRequestJobFactory.java index 79a3cfb467ad..4f328b12d922 100644 --- a/components/cronet/android/test/src/org/chromium/net/MockUrlRequestJobFactory.java +++ b/components/cronet/android/test/src/org/chromium/net/MockUrlRequestJobFactory.java @@ -24,6 +24,8 @@ public final class MockUrlRequestJobFactory { public static final String FAILED_URL = "http://mock.failed.request/-2"; + public static final String SUCCESS_BODY = "this is a text file\n"; + enum FailurePhase { START, READ_ASYNC, -- 2.11.4.GIT