[Cronet] JavaUrlRequest - Don't call close() on a user thread - it can do I/O.

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 638 matching lines @@
                     }
                 }));
             }
         });
     }
 
     private void processReadResult(int read, final ByteBuffer buffer) throws IOException {
         if (read != -1) {
             mCallbackAsync.onReadCompleted(mUrlResponseInfo, buffer);
         } else {
             mResponseChannel.close();

[mef, 2016/08/26 16:14:11]

Do we need to closeResponseChannel() here as well?

[Charles, 2016/08/26 18:15:51]

No, processReadResult is always called on mExecutor, and we want to capture any
exceptions thrown here as I/O errors.

             if (mState.compareAndSet(State.READING, State.COMPLETE)) {
                 fireDisconnect();
                 mCallbackAsync.onSucceeded(mUrlResponseInfo);
             }
         }
     }
 
     private void fireDisconnect() {
         final HttpURLConnection connection = mCurrentUrlConnection.getAndSet(null);
         if (connection != null) {
@@ skipping 122 matching lines @@
                 @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) {
-            closeQuietly(mResponseChannel);
+            closeResponseChannel();

[mef, 2016/08/25 22:23:54]

would it make sense to just call closeQuietly from the runnable below (similarly
in onFailed)?

[Charles, 2016/08/26 15:58:19]

No, because then we'd be doing I/O on the user's executor rather than our own. 

[mef, 2016/08/26 16:14:11]

Oh, I see. I've missed that those are different executors.

             mUserExecutor.execute(new Runnable() {
                 @Override
                 public void run() {
                     try {
                         mCallback.onCanceled(JavaUrlRequest.this, info);
                     } catch (Exception exception) {
                         Log.e(TAG, "Exception in onCanceled method", exception);
                     }
                 }
             });
         }
 
         void onSucceeded(final UrlResponseInfo info) {
             mUserExecutor.execute(new Runnable() {
                 @Override
                 public void run() {
                     try {
                         mCallback.onSucceeded(JavaUrlRequest.this, info);
                     } catch (Exception exception) {
                         Log.e(TAG, "Exception in onSucceeded method", exception);
                     }
                 }
             });
         }
 
         void onFailed(final UrlResponseInfo urlResponseInfo, final UrlRequestException e) {
-            closeQuietly(mResponseChannel);
+            closeResponseChannel();
             mUserExecutor.execute(new Runnable() {
                 @Override
                 public void run() {
                     try {
                         mCallback.onFailed(JavaUrlRequest.this, urlResponseInfo, e);
                     } catch (Exception exception) {
                         Log.e(TAG, "Exception in onFailed method", exception);
                     }
                 }
             });
         }
     }
 
-    private static void closeQuietly(Closeable closeable) {
+    private void closeResponseChannel() {
+        final Closeable closeable = mResponseChannel;
         if (closeable == null) {
             return;
         }
-        try {
-            closeable.close();
-        } catch (IOException e) {
-            e.printStackTrace();
-        }
+        mResponseChannel = null;
+        mExecutor.execute(new Runnable() {

[mef, 2016/08/26 16:14:11]

what if mExecutor rejects execution?

[Charles, 2016/08/26 18:15:51]

It can't, we use an unbounded thread pool that's never shut down for our I/O.
That's why usage of mExecutor doesn't check REE.

+            @Override
+            public void run() {
+                try {
+                    closeable.close();
+                } catch (IOException e) {
+                    e.printStackTrace();
+                }
+            }
+        });
     }
 }