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

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

Index: components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
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 41b0fc7b967f41ba0bff8b38e01cb2e2d69946ae..df2545a26cfbf1bf763ff61cd675e4e23bc45e43 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
@@ -73,13 +73,7 @@ public class CronetHttpURLConnection extends HttpURLConnection {
     @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.

         }
     }
@@ -455,13 +449,13 @@ public class CronetHttpURLConnection extends HttpURLConnection {
             }
             mResponseInfo = info;
             mRequest.cancel();
-            setResponseDataCompleted();
+            setResponseDataCompleted(null);
         }
 
         @Override
         public void onSucceeded(UrlRequest request, UrlResponseInfo info) {
             mResponseInfo = info;
-            setResponseDataCompleted();
+            setResponseDataCompleted(null);
         }
 
         @Override
@@ -473,16 +467,24 @@ public class CronetHttpURLConnection extends HttpURLConnection {
             }
             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();
         }