[Cronet] When connection is disconnected, InputStream#read should give an exception

components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java

 // Copyright 2014 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.urlconnection;
 
 import android.util.Pair;
 
 import org.chromium.base.Log;
 import org.chromium.net.CronetEngine;
@@ skipping 55 matching lines @@
         startRequest();
     }
 
     /**
      * Releases this connection so that its resources may be either reused or
      * closed.
      */
     @Override
     public void disconnect() {
         // Disconnect before connection is made should have no effect.
-        if (connected && mInputStream != null) {
-            try {
-                mInputStream.close();
-            } catch (IOException e) {
-                e.printStackTrace();
-            }
-            mInputStream = null;
+        if (connected) {
             mRequest.cancel();

[pauljensen, 2015/11/18 13:15:05]

This seems racy.  We cancel on this thread but the exception isn't put in place
until the onCanceled callback on another thread.  Seems like the old code was
racy too, it accessed mInputStream from this thread and from
setResponseDataCompleted (on another thread).

[xunjieli, 2015/11/18 21:14:07]

This is probably unavoidable. This HttpURLConnection class is not thread safe
(there is no thread safe guarantees anywhere). In crbug.com/550605, the user is
calling disconnect on a different thread. But assuming all methods are called
from the same thread, there won't be a race. I don't know what we can do here.

[pauljensen, 2015/11/19 04:51:08]

Can we make disconnect() call mMessageLoop.loop() after mRequest.cancel()? or
could we cut to the chase and avoid a lot of thread hops and just do:
   if (mInputStream != null) {
       mInputStream.setResponseDataCompleted(exception);
   }

[xunjieli, 2015/12/02 15:20:03]

I assume you meant mMessageLoop.quit(). I don't think we can, since we are not
inside the message loop, so we can't call quit.
 
I assume you meant we also quit the message loop here in addition to doing
mInputStream.setResponseDataCompleted(exception). Since if we already start
waiting, and disconnect is called on another thread,
mInputStream.setResponseDataCompleted(exception) alone won't terminate the wait.
Unfortunately, since we are not inside the message loop, it is illegal to call
quit.

CronetUrlRequest#Cancel is safe to call on a different thread, that is why I did
this here. I think it is a compromise that could be made. Since disconnect()
returns synchronously, client should not observe any delay.

[pauljensen, 2015/12/02 16:21:52]

No, I meant loop().  We know onCanceled() or some other callback will be posted
to mMessageLoop; I'm saying we should wait for it here.
I don't care if we quit the message loop or not.  I just want mInputStream to
hold the exception before disconnect() returns.  The current patch does not
accomplish this AFAIK.  It calls cancel() which will asynchronously later cause
onCanceled() to get posted to mMessageLoop but not executed.

[xunjieli, 2015/12/02 23:49:27]

I don't think we should call loop here. That might incur another round trip
regardless of whether the user actually cares. In most cases, when user call
disconnect, they don't actually care what happens. Calling into the message loop
on a potentially different thread is scary. We don't really know for sure what
the other thread is doing with the message loop.
I don't understand why we want to do that. Setting mInputStream's internal
states on a potentially different thread complicates the guarantees of
mInputStream's states. I'd like to think that all states changes happen on the
same thread. crbug.com/550605 calls disconnect on a different thread, and that
causes thread stall on the main HttpURLConnection thread. This patch tries to
solve that. But if we add mInputStream.setResponseDataCompleted here, we add a
potential pitfall where mInputStream is accessed from a different thread, which
I don't really like.

[pauljensen, 2015/12/03 04:04:27]

I really think we need to do something here.  This cancel() call is going to
result in onCanceled() being asynchronously posted to the message loop, but not
executed until some later operation or not executed at all.  I think all other
instances of posting to the message loop are followed by loop() calls, and
mMessageLoop is always left in an empty state.  I don't feel like we should be
implementing onCanceled() if we don't know when/if it's going to be executed. 
It seems to me like this function is leaving the request in an unpredictable
state that is ripe to cause bugs down the road.  For example if for whatever
reason the message loop is never run again, the exception won't ever reach
mInputStream and it will fail to be delivered to callers.

         }
     }
 
     /**
      * Returns the response message returned by the remote HTTP server.
      */
     @Override
     public String getResponseMessage() throws IOException {
         getResponse();
         return mResponseInfo.getHttpStatusText();
@@ skipping 354 matching lines @@
                 }
                 if (instanceFollowRedirects && sameProtocol) {
                     mRequest.followRedirect();
                     return;
                 }
             } catch (MalformedURLException e) {
                 // Ignored. Just cancel the request and not follow the redirect.
             }
             mResponseInfo = info;
             mRequest.cancel();
-            setResponseDataCompleted();
+            setResponseDataCompleted(null);
         }
 
         @Override
         public void onSucceeded(UrlRequest request, UrlResponseInfo info) {
             mResponseInfo = info;
-            setResponseDataCompleted();
+            setResponseDataCompleted(null);
         }
 
         @Override
         public void onFailed(
                 UrlRequest request, UrlResponseInfo info, UrlRequestException exception) {
             if (exception == null) {
                 throw new IllegalStateException(
                         "Exception cannot be null in onFailed.");
             }
             mResponseInfo = info;
             mException = exception;
-            setResponseDataCompleted();
+            setResponseDataCompleted(mException);
+        }
+
+        @Override
+        public void onCanceled(UrlRequest request, UrlResponseInfo info) {
+            mResponseInfo = info;
+            setResponseDataCompleted(new IOException("stream closed"));
         }
 
         /**
          * Notifies {@link #mInputStream} that transferring of response data has
          * completed.
+         * @param exception if not {@code null}, it is the exception to report when
+         *            caller tries to read more data.
          */
-        private void setResponseDataCompleted() {
+        private void setResponseDataCompleted(IOException exception) {
             if (mInputStream != null) {
-                mInputStream.setResponseDataCompleted();
+                mInputStream.setResponseDataCompleted(exception);
             }
             mMessageLoop.quit();
         }
     }
 
     /**
      * Blocks until the respone headers are received.
      */
     private void getResponse() throws IOException {
         // Check to see if enough data has been received.
@@ skipping 45 matching lines @@
     }
 
     /**
      * Returns whether the client has used {@link #setChunkedStreamingMode} to
      * set chunked encoding for upload.
      */
     private boolean isChunkedUpload() {
         return chunkLength > 0;
     }
 }