Cronet HttpURLConnection#getResponse might block when an upload fails |
|||||
Issue description
When consumer specifies a chunked upload or a fixed stream upload, Cronet's HttpURLConnection starts the request right away. If an error happens, we will surface that error as an IOException to consumer. However, if the consumer tries to call any other API (like getResponseCode(), getResponseMessage()) afterwards, we will spin up the message loop and wait forever!!
The correct behavior is to re-surface the same error that we reported to consumer earlier.
An repro case:
public void testGetResponseAfterWriteFailed() throws Exception {
URL url = new URL(NativeTestServer.getEchoBodyURL());
NativeTestServer.shutdownNativeTestServer();
HttpURLConnection connection = (HttpURLConnection) url.openConnection();
connection.setDoOutput(true);
connection.setRequestMethod("POST");
connection.setFixedLengthStreamingMode(1);
OutputStream out = connection.getOutputStream();
try {
out.write(1);
} catch (IOException e) {
// Expected.
if (!testingSystemHttpURLConnection()) {
UrlRequestException requestException = (UrlRequestException) e;
assertEquals(UrlRequestException.ERROR_CONNECTION_REFUSED,
requestException.getErrorCode());
}
}
try {
connection.getResponseCode(); <--- We block here forever!
} catch (IOException e) {
// Expected.
if (!testingSystemHttpURLConnection()) {
UrlRequestException requestException = (UrlRequestException) e;
assertEquals(UrlRequestException.ERROR_CONNECTION_REFUSED,
requestException.getErrorCode());
}
}
}
The fix is a one-liner change.
,
Jul 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16 commit 7fbfe4fe8ff451d1f65ac574a2304d383aaeec16 Author: xunjieli <xunjieli@chromium.org> Date: Mon Jul 25 13:59:49 2016 [Cronet] Mark request as complete when OutputStream fails When consumer specifies a chunked upload or a fixed stream upload, Cronet's HttpURLConnection starts the request right away. If an error happens, we will surface that error as an IOException to consumer. However, if the consumer tries to call any other API (like getResponseCode(), getResponseMessage()) afterwards, we will spin up the message loop and wait forever. This CL marks the request as complete if an error occurred in upload, so getResponseCode() etc. do not spin up the message loop to wait for response. R=pauljensen@chromium.org BUG= 630664 Review-Url: https://codereview.chromium.org/2173923002 Cr-Commit-Position: refs/heads/master@{#407468} [modify] https://crrev.com/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java [modify] https://crrev.com/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java [modify] https://crrev.com/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java [modify] https://crrev.com/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java [modify] https://crrev.com/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java [modify] https://crrev.com/7fbfe4fe8ff451d1f65ac574a2304d383aaeec16/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java
,
Jul 25 2016
,
Jul 25 2016
Requesting merge into M53. This CL doesn't impact Chrome. The code is only compiled into Cronet. Thanks!
,
Jul 25 2016
Your change meets the bar and is auto-approved for M53 (branch: 2785)
,
Jul 25 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dcd8f0f911957e45232d22e85cac0e381dc26d35 commit dcd8f0f911957e45232d22e85cac0e381dc26d35 Author: xunjieli <xunjieli@chromium.org> Date: Mon Jul 25 18:33:20 2016 [Cronet] Mark request as complete when OutputStream fails When consumer specifies a chunked upload or a fixed stream upload, Cronet's HttpURLConnection starts the request right away. If an error happens, we will surface that error as an IOException to consumer. However, if the consumer tries to call any other API (like getResponseCode(), getResponseMessage()) afterwards, we will spin up the message loop and wait forever. This CL marks the request as complete if an error occurred in upload, so getResponseCode() etc. do not spin up the message loop to wait for response. TBR=pauljensen@chromium.org NOTRY=true NOPRESUBMIT=true BUG= 630664 Review-Url: https://codereview.chromium.org/2173923002 Cr-Commit-Position: refs/heads/master@{#407468} (cherry picked from commit 7fbfe4fe8ff451d1f65ac574a2304d383aaeec16) Review-Url: https://codereview.chromium.org/2175403002 Cr-Commit-Position: refs/branch-heads/2785@{#333} Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382} [modify] https://crrev.com/dcd8f0f911957e45232d22e85cac0e381dc26d35/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java [modify] https://crrev.com/dcd8f0f911957e45232d22e85cac0e381dc26d35/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java [modify] https://crrev.com/dcd8f0f911957e45232d22e85cac0e381dc26d35/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java [modify] https://crrev.com/dcd8f0f911957e45232d22e85cac0e381dc26d35/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java [modify] https://crrev.com/dcd8f0f911957e45232d22e85cac0e381dc26d35/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java [modify] https://crrev.com/dcd8f0f911957e45232d22e85cac0e381dc26d35/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by xunji...@chromium.org
, Jul 22 2016