Improve error handling in JavaUrlRequest

components/cronet/android/api/src/org/chromium/net/JavaUrlRequest.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.annotation.TargetApi;
 import android.net.TrafficStats;
 import android.os.Build;
 import android.util.Log;
@@ skipping 272 matching lines @@
             this.mUrlConnection = urlConnection;
             this.mUploadProvider = provider;
         }
 
         @Override
         public void onReadSucceeded(final boolean finalChunk) {
             if (!mSinkState.compareAndSet(SinkState.AWAITING_READ_RESULT, SinkState.UPLOADING)) {
                 throw new IllegalStateException(
                         "Not expecting a read result, expecting: " + mSinkState.get());
             }
-            mExecutor.execute(errorSetting(State.STARTED, new CheckedRunnable() {
+            mExecutor.execute(errorSetting(new CheckedRunnable() {
                 @Override
                 public void run() throws Exception {
                     mBuffer.flip();
                     if (mTotalBytes != -1 && mTotalBytes - mWrittenBytes < mBuffer.remaining()) {
                         enterUploadErrorState(new IllegalArgumentException(String.format(
                                 "Read upload data length %d exceeds expected length %d",
                                 mWrittenBytes + mBuffer.remaining(), mTotalBytes)));
                         return;
                     }
                     while (mBuffer.hasRemaining()) {
@@ skipping 33 matching lines @@
         public void onReadError(Exception exception) {
             enterUploadErrorState(exception);
         }
 
         @Override
         public void onRewindError(Exception exception) {
             enterUploadErrorState(exception);
         }
 
         void startRead() {
-            mExecutor.execute(errorSetting(State.STARTED, new CheckedRunnable() {
+            mExecutor.execute(errorSetting(new CheckedRunnable() {
                 @Override
                 public void run() throws Exception {
                     if (mOutputChannel == null) {
                         mAdditionalStatusDetails = Status.CONNECTING;
                         mUrlConnection.connect();
                         mAdditionalStatusDetails = Status.SENDING_REQUEST;
                         mOutputChannel = Channels.newChannel(mUrlConnection.getOutputStream());
                     }
                     mSinkState.set(SinkState.AWAITING_READ_RESULT);
                     executeOnUploadExecutor(new CheckedRunnable() {
@@ skipping 67 matching lines @@
         mAdditionalStatusDetails = Status.CONNECTING;
         transitionStates(State.NOT_STARTED, State.STARTED, new Runnable() {
             @Override
             public void run() {
                 mUrlChain.add(mCurrentUrl);
                 fireOpenConnection();
             }
         });
     }
 
-    private void enterErrorState(State previousState, final UrlRequestException error) {
-        if (mState.compareAndSet(previousState, State.ERROR)) {
+    private void enterErrorState(final UrlRequestException error) {
+        if (setTerminalState(State.ERROR)) {
             fireDisconnect();
             fireCloseUploadDataProvider();
             mCallbackAsync.onFailed(mUrlResponseInfo, error);
         }
     }
 
+    private boolean setTerminalState(State error) {
+        while (true) {

[xunjieli, 2016/09/02 21:05:01]

I don't understand the need for a while-true loop. Could you elaborate?

[Charles, 2016/09/02 21:08:36]

We're doing a compare-and-set continuously until we either observe a terminal
state, or successfully set a terminal state. The loop only continues if we
observe a non-terminal state, like READING, but when we try to compare-and-set
READING to ERROR, the current state changed to something else. Then we repeat
the process. In practice, this loop will almost never run more than once, and it
can't run forever because the only way to have the CAS fail is for the system to
make progress towards a completed state.

[xunjieli, 2016/09/02 21:41:14]

Acknowledged.

+            State oldState = mState.get();
+            switch (oldState) {
+                case NOT_STARTED:
+                    throw new IllegalStateException("Can't enter error state before start");
+                case ERROR: // fallthrough
+                case COMPLETE: // fallthrough
+                case CANCELLED:
+                    return false; // Already in a terminal state
+                default: {
+                    if (mState.compareAndSet(oldState, error)) {
+                        return true;
+                    }
+                }
+            }
+        }
+    }
+
     /** Ends the request with an error, caused by an exception thrown from user code. */
-    private void enterUserErrorState(State previousState, final Throwable error) {
-        enterErrorState(previousState,
+    private void enterUserErrorState(final Throwable error) {
+        enterErrorState(
                 new UrlRequestException("Exception received from UrlRequest.Callback", error));
     }
 
     /** Ends the request with an error, caused by an exception thrown from user code. */
     private void enterUploadErrorState(final Throwable error) {
-        enterErrorState(State.STARTED,
+        enterErrorState(
                 new UrlRequestException("Exception received from UploadDataProvider", error));
     }
 
-    private void enterCronetErrorState(State previousState, final Throwable error) {
+    private void enterCronetErrorState(final Throwable error) {
         // TODO(clm) mapping from Java exception (UnknownHostException, for example) to net error
         // code goes here.
-        enterErrorState(previousState, new UrlRequestException("System error", error));
+        enterErrorState(new UrlRequestException("System error", error));
     }
 
     /**
      * Atomically swaps from the expected state to a new state. If the swap fails, and it's not
      * due to an earlier error or cancellation, throws an exception.
      *
      * @param afterTransition Callback to run after transition completes successfully.
      */
     private void transitionStates(State expected, State newState, Runnable afterTransition) {
         if (!mState.compareAndSet(expected, newState)) {
@@ skipping 14 matching lines @@
             public void run() {
                 mCurrentUrl = mPendingRedirectUrl;
                 mPendingRedirectUrl = null;
                 fireOpenConnection();
             }
         });
     }
 
     private void fireGetHeaders() {
         mAdditionalStatusDetails = Status.WAITING_FOR_RESPONSE;
-        mExecutor.execute(errorSetting(State.STARTED, new CheckedRunnable() {
+        mExecutor.execute(errorSetting(new CheckedRunnable() {
             @Override
             public void run() throws Exception {
                 HttpURLConnection connection = mCurrentUrlConnection.get();
                 if (connection == null) {
                     return; // We've been cancelled
                 }
                 final List<Map.Entry<String, String>> headerList = new ArrayList<>();
                 String selectedTransport = "http/1.1";
                 String headerKey;
                 for (int i = 0; (headerKey = connection.getHeaderFieldKey(i)) != null; i++) {
@@ skipping 58 matching lines @@
                             public void run() {
                                 mCallbackAsync.onRedirectReceived(
                                         mUrlResponseInfo, mPendingRedirectUrl);
                             }
                         });
             }
         });
     }
 
     private void fireOpenConnection() {
-        mExecutor.execute(errorSetting(State.STARTED, new CheckedRunnable() {
+        mExecutor.execute(errorSetting(new CheckedRunnable() {
             @Override
             public void run() throws Exception {
                 // If we're cancelled, then our old connection will be disconnected for us and
                 // we shouldn't open a new one.
                 if (mState.get() == State.CANCELLED) {
                     return;
                 }
 
                 final URL url = new URL(mCurrentUrl);
                 HttpURLConnection newConnection = (HttpURLConnection) url.openConnection();
@@ skipping 18 matching lines @@
                     dataSink.start(mUrlChain.size() == 1);
                 } else {
                     mAdditionalStatusDetails = Status.CONNECTING;
                     newConnection.connect();
                     fireGetHeaders();
                 }
             }
         }));
     }
 
-    private Runnable errorSetting(final State expectedState, final CheckedRunnable delegate) {
+    private Runnable errorSetting(final CheckedRunnable delegate) {
         return new Runnable() {
             @Override
             public void run() {
                 try {
                     delegate.run();
                 } catch (Throwable t) {
-                    enterCronetErrorState(expectedState, t);
+                    enterCronetErrorState(t);
                 }
             }
         };
     }
 
-    private Runnable userErrorSetting(final State expectedState, final CheckedRunnable delegate) {
+    private Runnable userErrorSetting(final CheckedRunnable delegate) {
         return new Runnable() {
             @Override
             public void run() {
                 try {
                     delegate.run();
                 } catch (Throwable t) {
-                    enterUserErrorState(expectedState, t);
+                    enterUserErrorState(t);
                 }
             }
         };
     }
 
     private Runnable uploadErrorSetting(final CheckedRunnable delegate) {
         return new Runnable() {
             @Override
             public void run() {
                 try {
                     delegate.run();
                 } catch (Throwable t) {
                     enterUploadErrorState(t);
                 }
             }
         };
     }
 
     private interface CheckedRunnable { void run() throws Exception; }
 
     @Override
     public void read(final ByteBuffer buffer) {
         Preconditions.checkDirect(buffer);
         Preconditions.checkHasRemaining(buffer);
         transitionStates(State.AWAITING_READ, State.READING, new Runnable() {
             @Override
             public void run() {
-                mExecutor.execute(errorSetting(State.READING, new CheckedRunnable() {
+                mExecutor.execute(errorSetting(new CheckedRunnable() {
                     @Override
                     public void run() throws Exception {
                         int read = mResponseChannel.read(buffer);
                         processReadResult(read, buffer);
                     }
                 }));
             }
         });
     }
 
@@ skipping 104 matching lines @@
 
         void sendStatus(final StatusListener listener, final int status) {
             mUserExecutor.execute(new Runnable() {
                 @Override
                 public void run() {
                     listener.onStatus(status);
                 }
             });
         }
 
-        void execute(State currentState, CheckedRunnable runnable) {
+        void execute(CheckedRunnable runnable) {
             try {
-                mUserExecutor.execute(userErrorSetting(currentState, runnable));
+                mUserExecutor.execute(userErrorSetting(runnable));
             } catch (RejectedExecutionException e) {
-                enterErrorState(currentState,
-                        new UrlRequestException("Exception posting task to executor", e));
+                enterErrorState(new UrlRequestException("Exception posting task to executor", e));
             }
         }
 
         void onRedirectReceived(final UrlResponseInfo info, final String newLocationUrl) {
-            execute(State.AWAITING_FOLLOW_REDIRECT, new CheckedRunnable() {
+            execute(new CheckedRunnable() {
                 @Override
                 public void run() throws Exception {
                     mCallback.onRedirectReceived(JavaUrlRequest.this, info, newLocationUrl);
                 }
             });
         }
 
         void onResponseStarted(UrlResponseInfo info) {
-            if (mState.compareAndSet(State.STARTED, State.AWAITING_READ)) {
-                execute(State.AWAITING_READ, new CheckedRunnable() {
-                    @Override
-                    public void run() throws Exception {
+            execute(new CheckedRunnable() {
+                @Override
+                public void run() throws Exception {
+                    if (mState.compareAndSet(State.STARTED, State.AWAITING_READ)) {
                         mCallback.onResponseStarted(JavaUrlRequest.this, mUrlResponseInfo);
                     }
-                });
-            }
+                }
+            });
         }
 
         void onReadCompleted(final UrlResponseInfo info, final ByteBuffer byteBuffer) {
-            if (mState.compareAndSet(State.READING, State.AWAITING_READ)) {
-                execute(State.AWAITING_READ, new CheckedRunnable() {
-                    @Override
-                    public void run() throws Exception {
+            execute(new CheckedRunnable() {
+                @Override
+                public void run() throws Exception {
+                    if (mState.compareAndSet(State.READING, State.AWAITING_READ)) {
                         mCallback.onReadCompleted(JavaUrlRequest.this, info, byteBuffer);
                     }
-                });
-            }
+                }
+            });
         }
 
         void onCanceled(final UrlResponseInfo info) {
             closeResponseChannel();
             mUserExecutor.execute(new Runnable() {
                 @Override
                 public void run() {
                     try {
                         mCallback.onCanceled(JavaUrlRequest.this, info);
                     } catch (Exception exception) {
@@ skipping 99 matching lines @@
                     // Can't throw directly from here, since the delegate executor could catch this
                     // exception.
                     mExecutedInline = new InlineExecutionProhibitedException();
                     return;
                 }
                 mCommand.run();
             }
         }
     }
 }