Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1413303006:
[Cronet] When connection is disconnected, InputStream#read should give an exception (Closed)
|
Created:
5 years, 1 month ago by xunjieli Modified:
5 years ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] When connection is disconnected, InputStream#read should give an exception
[Cronet] When connection is disconnected before response
body is fully read, future InputStream#read should give an
appropriate exception. On the other hand, when disconnect
is called after data is read, InputStream#read should
only return EOF and not cause an exception.
This is done to match InputStream's documentation.
This CL also refactors methods shared between test classes
into a util class.
BUG=550605
Committed: https://crrev.com/b6e92d4990752e5852ad05b8409f989073ecf5eb
Cr-Commit-Position: refs/heads/master@{#363496}
Patch Set 1 #Patch Set 2 : Propagate exception to input stream #
Total comments: 2
Patch Set 3 : use @code #
Total comments: 14
Patch Set 4 : Address Paul's comments #Messages
Total messages: 30 (9 generated)
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org
Paul, PTAL.
Description was changed from ========== [Cronet] Do not run message loop to get more data when the connection is disconnected When HttpURLConnection.disconnect is called when we have not finished reading, we should not spin up message loop to get more data, because this will block the current thread. BUG=550605 ========== to ========== [Cronet] Do not run message loop to get more data when the connection is disconnected When HttpURLConnection.disconnect is called when we have not finished reading, we should not spin up message loop to get more data, because this will block the current thread. This CL also refactors methods shared between test classes into a util class. BUG=550605 ==========
This looks like a big improvement in behavior. Can we throw an exception too? I see this as the documented behavior: http://developer.android.com/reference/java/io/InputStream.html#read() "Returns -1 if the end of the stream has been reached" "Throws IOException if the stream is closed" I don't think we've hit the end of stream, I think in this case the stream is closed.
On 2015/11/09 12:06:16, pauljensen wrote: > This looks like a big improvement in behavior. > Can we throw an exception too? I see this as the documented behavior: > http://developer.android.com/reference/java/io/InputStream.html#read() > "Returns -1 if the end of the stream has been reached" > "Throws IOException if the stream is closed" > I don't think we've hit the end of stream, I think in this case the stream is > closed. Good catch! Done. Thanks. PTAL.
What about Cronet*OutputStream? what should they do if disconnected() was already called? https://codereview.chromium.org/1413303006/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/1413303006/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:62: * @param exception if not null, it is the exception to throw when caller here and elsewhere: null->{@code null}
Good question. I am not sure. I think we should write some tests to find out what the default implementation does. But I agree that we will need to take care of the cancel case there to make sure write operations won't block. https://codereview.chromium.org/1413303006/diff/20001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/1413303006/diff/20001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:62: * @param exception if not null, it is the exception to throw when caller On 2015/11/10 15:55:21, pauljensen wrote: > here and elsewhere: > null->{@code null} Done.
Description was changed from ========== [Cronet] Do not run message loop to get more data when the connection is disconnected When HttpURLConnection.disconnect is called when we have not finished reading, we should not spin up message loop to get more data, because this will block the current thread. This CL also refactors methods shared between test classes into a util class. BUG=550605 ========== to ========== [Cronet] Do not run message loop to get more data when the connection is disconnected When HttpURLConnection.disconnect is called when we have not finished reading, we should not spin up message loop to get more data, because this will block the current thread. Instead surface any exception that we received when caller tries to read more data. This CL also refactors methods shared between test classes into a util class. BUG=550605 ==========
Description was changed from ========== [Cronet] Do not run message loop to get more data when the connection is disconnected When HttpURLConnection.disconnect is called when we have not finished reading, we should not spin up message loop to get more data, because this will block the current thread. Instead surface any exception that we received when caller tries to read more data. This CL also refactors methods shared between test classes into a util class. BUG=550605 ========== to ========== [Cronet] When connection is disconnected, InputStream#read should give an exception [Cronet] When connection is disconnected before response body is fully read, future InputStream#read should give an appropriate exception. On the other hand, when disconnect is called after data is read, InputStream#read should only return EOF and not cause an exception. This is done to match InputStream's documentation. This CL also refactors methods shared between test classes into a util class. BUG=550605 ==========
On 2015/11/10 16:16:59, xunjieli wrote: > Good question. I am not sure. I think we should write some tests to find out > what the default implementation does. But I agree that we will need to take care > of the cancel case there to make sure write operations won't block. Do you want to do this in this CL?
I think we can keep them separate so we can spend more time in making sure the test cases are comprehensive. On Tue, Nov 10, 2015, 12:02 PM <pauljensen@chromium.org> wrote: > On 2015/11/10 16:16:59, xunjieli wrote: > > Good question. I am not sure. I think we should write some tests to find > > out > > what the default implementation does. But I agree that we will need to > > take > care > > of the cancel case there to make sure write operations won't block. > > Do you want to do this in this CL? > > https://codereview.chromium.org/1413303006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:77: mRequest.cancel(); 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).
https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:77: mRequest.cancel(); On 2015/11/18 13:15:05, pauljensen wrote: > 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). 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.
https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:77: mRequest.cancel(); On 2015/11/18 21:14:07, xunjieli wrote: > On 2015/11/18 13:15:05, pauljensen wrote: > > 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). > > 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. 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); }
PTAL https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:77: mRequest.cancel(); On 2015/11/19 04:51:08, pauljensen wrote: > On 2015/11/18 21:14:07, xunjieli wrote: > > On 2015/11/18 13:15:05, pauljensen wrote: > > > 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). > > > > 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. > > Can we make disconnect() call mMessageLoop.loop() after mRequest.cancel()? or 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. > could we cut to the chase and avoid a lot of thread hops and just do: > if (mInputStream != null) { > mInputStream.setResponseDataCompleted(exception); > } 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.
https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:77: mRequest.cancel(); On 2015/12/02 15:20:03, xunjieli wrote: > On 2015/11/19 04:51:08, pauljensen wrote: > > On 2015/11/18 21:14:07, xunjieli wrote: > > > On 2015/11/18 13:15:05, pauljensen wrote: > > > > 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). > > > > > > 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. > > > > Can we make disconnect() call mMessageLoop.loop() after mRequest.cancel()? or > 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. 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. > > > could we cut to the chase and avoid a lot of thread hops and just do: > > if (mInputStream != null) { > > mInputStream.setResponseDataCompleted(exception); > > } > 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. 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. > > 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.
https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:77: mRequest.cancel(); On 2015/12/02 16:21:52, pauljensen wrote: > On 2015/12/02 15:20:03, xunjieli wrote: > > On 2015/11/19 04:51:08, pauljensen wrote: > > > On 2015/11/18 21:14:07, xunjieli wrote: > > > > On 2015/11/18 13:15:05, pauljensen wrote: > > > > > 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). > > > > > > > > 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. > > > > > > Can we make disconnect() call mMessageLoop.loop() after mRequest.cancel()? > or > > 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. > 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 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. > > > > > could we cut to the chase and avoid a lot of thread hops and just do: > > > if (mInputStream != null) { > > > mInputStream.setResponseDataCompleted(exception); > > > } > > 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. > 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. 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. > It calls cancel() which will asynchronously later cause > onCanceled() to get posted to mMessageLoop but not executed. > > > > > 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. >
https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java:77: mRequest.cancel(); On 2015/12/02 23:49:27, xunjieli wrote: > On 2015/12/02 16:21:52, pauljensen wrote: > > On 2015/12/02 15:20:03, xunjieli wrote: > > > On 2015/11/19 04:51:08, pauljensen wrote: > > > > On 2015/11/18 21:14:07, xunjieli wrote: > > > > > On 2015/11/18 13:15:05, pauljensen wrote: > > > > > > 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). > > > > > > > > > > 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. > > > > > > > > Can we make disconnect() call mMessageLoop.loop() after mRequest.cancel()? > > or > > > 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. > > 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 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. > > > > > > > could we cut to the chase and avoid a lot of thread hops and just do: > > > > if (mInputStream != null) { > > > > mInputStream.setResponseDataCompleted(exception); > > > > } > > > 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. > > 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. > 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. > > > It calls cancel() which will asynchronously later cause > > onCanceled() to get posted to mMessageLoop but not executed. > > > > > > > > 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. > > > 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.
lgtm with nits https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:63: * tries to read more data than what is buffered. nit: I'm a little confused by the "more data than what is buffered" comment. This function clears the buffer, so any further request will be for more data than is buffered, correct? Can we just leave it as "...tries to read more data." https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/TestUtil.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/TestUtil.java:28: while ((b = in.read()) != -1) { nit: I've heard you can do this without looping with new java.util.Scanner(in).useDelimiter("\\A").next() but I've never tried https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/TestUtil.java:41: for (int j = 0; j < UPLOAD_DATA.length; j++) { nit: might be good to replace this inner loop with System.arrayCopy
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/urlconnection/CronetInputStream.java:63: * tries to read more data than what is buffered. On 2015/12/07 15:20:07, pauljensen wrote: > nit: I'm a little confused by the "more data than what is buffered" comment. > This function clears the buffer, so any further request will be for more data > than is buffered, correct? Can we just leave it as "...tries to read more > data." Done. https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/TestUtil.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/TestUtil.java:28: while ((b = in.read()) != -1) { On 2015/12/07 15:20:08, pauljensen wrote: > nit: I've heard you can do this without looping with new > java.util.Scanner(in).useDelimiter("\\A").next() but I've never tried I tried using a wrapper before (to read line by line), but mmenke@ suggested I read one byte at a time. This is to know exactly what function of the CronetInputStream we are exericising. (I have some tests, where I read a couple of bytes at a time.) I think I will leave this one as it is. https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/TestUtil.java:41: for (int j = 0; j < UPLOAD_DATA.length; j++) { On 2015/12/07 15:20:08, pauljensen wrote: > nit: might be good to replace this inner loop with System.arrayCopy Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/1413303006/#ps80001 (title: "Address Paul's comments")
https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/TestUtil.java (right): https://codereview.chromium.org/1413303006/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/TestUtil.java:28: while ((b = in.read()) != -1) { On 2015/12/07 15:53:12, xunjieli wrote: > On 2015/12/07 15:20:08, pauljensen wrote: > > nit: I've heard you can do this without looping with new > > java.util.Scanner(in).useDelimiter("\\A").next() but I've never tried > > I tried using a wrapper before (to read line by line), but mmenke@ suggested I > read one byte at a time. This is to know exactly what function of the > CronetInputStream we are exericising. (I have some tests, where I read a couple > of bytes at a time.) I think I will leave this one as it is. sgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413303006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413303006/80001
lgtm
Message was sent while issue was closed.
Description was changed from ========== [Cronet] When connection is disconnected, InputStream#read should give an exception [Cronet] When connection is disconnected before response body is fully read, future InputStream#read should give an appropriate exception. On the other hand, when disconnect is called after data is read, InputStream#read should only return EOF and not cause an exception. This is done to match InputStream's documentation. This CL also refactors methods shared between test classes into a util class. BUG=550605 ========== to ========== [Cronet] When connection is disconnected, InputStream#read should give an exception [Cronet] When connection is disconnected before response body is fully read, future InputStream#read should give an appropriate exception. On the other hand, when disconnect is called after data is read, InputStream#read should only return EOF and not cause an exception. This is done to match InputStream's documentation. This CL also refactors methods shared between test classes into a util class. BUG=550605 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] When connection is disconnected, InputStream#read should give an exception [Cronet] When connection is disconnected before response body is fully read, future InputStream#read should give an appropriate exception. On the other hand, when disconnect is called after data is read, InputStream#read should only return EOF and not cause an exception. This is done to match InputStream's documentation. This CL also refactors methods shared between test classes into a util class. BUG=550605 ========== to ========== [Cronet] When connection is disconnected, InputStream#read should give an exception [Cronet] When connection is disconnected before response body is fully read, future InputStream#read should give an appropriate exception. On the other hand, when disconnect is called after data is read, InputStream#read should only return EOF and not cause an exception. This is done to match InputStream's documentation. This CL also refactors methods shared between test classes into a util class. BUG=550605 Committed: https://crrev.com/b6e92d4990752e5852ad05b8409f989073ecf5eb Cr-Commit-Position: refs/heads/master@{#363496} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b6e92d4990752e5852ad05b8409f989073ecf5eb Cr-Commit-Position: refs/heads/master@{#363496} |
