Cronet HttpURLConnection should rethrow IOException, not IllegalStateException |
|||
Issue description
If Cronet's getHeaderField()'s call to getResponse() throws, then later calls to getResponseCode() shouldn't throw IllegalStateException("Cannot run loop as an exception has occurred previously."), but should instead rethrow an IOException with the cause set to the other IOException.
Here's a reduced test case:
import java.net.HttpURLConnection;
import java.net.URL;
public class HelloWorld {
public static void main(String[] args) throws Exception {
HttpURLConnection c = (HttpURLConnection)new URL("http://lkjlsnlwekfwwew").openConnection();
System.out.println("header = " + c.getHeaderField("Set-Cookie"));
System.out.println("res code = " + c.getResponseCode());
}
}
And here's what JDK does:
header = null
Exception in thread "main" java.net.UnknownHostException: lkjlsnlwekfwwew
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
at sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:1944)
at sun.net.www.protocol.http.HttpURLConnection$10.run(HttpURLConnection.java:1939)
at java.security.AccessController.doPrivileged(Native Method)
at sun.net.www.protocol.http.HttpURLConnection.getChainedException(HttpURLConnection.java:1938)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1508)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1492)
at java.net.HttpURLConnection.getResponseCode(HttpURLConnection.java:480)
at HelloWorld.main(HelloWorld.java:8)
Caused by: java.net.UnknownHostException: lkjlsnlwekfwwew
at java.net.AbstractPlainSocketImpl.connect(AbstractPlainSocketImpl.java:184)
at java.net.SocksSocketImpl.connect(SocksSocketImpl.java:392)
at java.net.Socket.connect(Socket.java:589)
at java.net.Socket.connect(Socket.java:538)
at sun.net.NetworkClient.doConnect(NetworkClient.java:180)
at sun.net.www.http.HttpClient.openServer(HttpClient.java:463)
at sun.net.www.http.HttpClient.openServer(HttpClient.java:558)
at sun.net.www.http.HttpClient.<init>(HttpClient.java:242)
at sun.net.www.http.HttpClient.New(HttpClient.java:339)
at sun.net.www.http.HttpClient.New(HttpClient.java:357)
at sun.net.www.protocol.http.HttpURLConnection.getNewHttpClient(HttpURLConnection.java:1220)
at sun.net.www.protocol.http.HttpURLConnection.plainConnect0(HttpURLConnection.java:1156)
at sun.net.www.protocol.http.HttpURLConnection.plainConnect(HttpURLConnection.java:1050)
at sun.net.www.protocol.http.HttpURLConnection.connect(HttpURLConnection.java:984)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(HttpURLConnection.java:1564)
at sun.net.www.protocol.http.HttpURLConnection.getInputStream(HttpURLConnection.java:1492)
at sun.net.www.protocol.http.HttpURLConnection.getHeaderField(HttpURLConnection.java:3000)
at HelloWorld.main(HelloWorld.java:7)
Internal bug b/111663024
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fa9d5059610a89c91414da10a66c38ff8cda30ef commit fa9d5059610a89c91414da10a66c38ff8cda30ef Author: Paul Jensen <pauljensen@chromium.org> Date: Wed Jul 25 16:39:51 2018 [Cronet] Fix HttpURLConnection to not throw IllegalStateExceptions Some methods like HttpURLConnection.getHeader() hide IOExceptions, but later calls to getResponseCode() would then throw unexpected IllegalStateExceptions. Instead getResponseCode() should rethrow the previously hidden IOException. Bug: 866911 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Change-Id: Icc533d4a69875af5570a6f793463754d1bf477ab Reviewed-on: https://chromium-review.googlesource.com/1148475 Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Paul Jensen <pauljensen@chromium.org> Cr-Commit-Position: refs/heads/master@{#577931} [modify] https://crrev.com/fa9d5059610a89c91414da10a66c38ff8cda30ef/components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java [modify] https://crrev.com/fa9d5059610a89c91414da10a66c38ff8cda30ef/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java [modify] https://crrev.com/fa9d5059610a89c91414da10a66c38ff8cda30ef/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/MessageLoopTest.java
,
Jul 26
My test flaked once: https://ci.chromium.org/buildbot/chromium.android/Android%20Cronet%20Marshmallow%2064bit%20Builder/23249 It's probably a little racy because it doesn't check if the HttpURLConnection is waiting to catch the thread interrupt.
,
Jul 26
Hmm bot flake happened on a Nexus 5X running Marshmallow. I just ran the test 440 times on a Nexus 5X running Marshmallow but it passed every time.
,
Jul 26
I wonder whether connect() could make the usage thread unsafe.
--------------------------
final HttpURLConnection connection = (HttpURLConnection)
url.openConnection(); // connect() is non-blocking.
connection.connect();
-------------------------
The connect() will start the request on current thread. Should we get rid
of it, and rely on getHeaderField() to start the request on that separate
thread?
,
Jul 26
I believe it should be fine to call different non-thread-safe functions on different threads as long as they are never called concurrently. In my test they are not called concurrently. One part of my test that races is the call to getHeaderField() and the call to interrupt(). I don't know if the interrupt() operation is a sticky one or an ephemeral one (i.e. do later calls to block/wait throw InterruptedException if the interrupt() call was made previously, or does interrupt() only interrupt blocking operations that are already blocking). Actually I can confirm this race can cause the test to flake: I added a sleep before getHeaderField() and it always fails.
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2bf1cdb05f02bc83ed3624eaa7caeaf9942bff6f commit 2bf1cdb05f02bc83ed3624eaa7caeaf9942bff6f Author: Paul Jensen <pauljensen@chromium.org> Date: Fri Jul 27 14:51:00 2018 [Cronet] Deflake CronetHttpURLConnectionTest#testIOExceptionInterruptRethrown Don't test system impl as it doesn't always respond to interrupt. Attempt to reduce race in test. Bug: 866911 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Change-Id: I74dc6c4ffdec4654cb0e0ab2912fb54a8564d717 Reviewed-on: https://chromium-review.googlesource.com/1152988 Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Paul Jensen <pauljensen@chromium.org> Cr-Commit-Position: refs/heads/master@{#578639} [modify] https://crrev.com/2bf1cdb05f02bc83ed3624eaa7caeaf9942bff6f/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0d9cd0954c54c079d5176a9748ea667c4d929e50 commit 0d9cd0954c54c079d5176a9748ea667c4d929e50 Author: Paul Jensen <pauljensen@chromium.org> Date: Mon Jul 30 23:52:45 2018 [Cronet] Fix HttpURLConnection to not throw IllegalStateExceptions Some methods like HttpURLConnection.getHeader() hide IOExceptions, but later calls to getResponseCode() would then throw unexpected IllegalStateExceptions. Instead getResponseCode() should rethrow the previously hidden IOException. This change does not affect Chrome, only Cronet, so it is being merged back without typical merge approval. Bug: 866911 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Change-Id: Icc533d4a69875af5570a6f793463754d1bf477ab Reviewed-on: https://chromium-review.googlesource.com/1148475 Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Paul Jensen <pauljensen@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577931}(cherry picked from commit fa9d5059610a89c91414da10a66c38ff8cda30ef) Reviewed-on: https://chromium-review.googlesource.com/1155765 Reviewed-by: Paul Jensen <pauljensen@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#255} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/0d9cd0954c54c079d5176a9748ea667c4d929e50/components/cronet/android/java/src/org/chromium/net/urlconnection/MessageLoop.java [modify] https://crrev.com/0d9cd0954c54c079d5176a9748ea667c4d929e50/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java [modify] https://crrev.com/0d9cd0954c54c079d5176a9748ea667c4d929e50/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/MessageLoopTest.java
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4518ad8a479b381f4a4ab820299785c6dbf7a307 commit 4518ad8a479b381f4a4ab820299785c6dbf7a307 Author: Paul Jensen <pauljensen@chromium.org> Date: Mon Jul 30 23:55:30 2018 [Cronet] Deflake CronetHttpURLConnectionTest#testIOExceptionInterruptRethrown Don't test system impl as it doesn't always respond to interrupt. Attempt to reduce race in test. This change does not affect Chrome, only Cronet, so it is being merged back without typical merge approval. Bug: 866911 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester Change-Id: I74dc6c4ffdec4654cb0e0ab2912fb54a8564d717 Reviewed-on: https://chromium-review.googlesource.com/1152988 Reviewed-by: Helen Li <xunjieli@chromium.org> Commit-Queue: Paul Jensen <pauljensen@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#578639}(cherry picked from commit 2bf1cdb05f02bc83ed3624eaa7caeaf9942bff6f) Reviewed-on: https://chromium-review.googlesource.com/1155766 Reviewed-by: Paul Jensen <pauljensen@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#256} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/4518ad8a479b381f4a4ab820299785c6dbf7a307/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetHttpURLConnectionTest.java
,
Jul 30
|
|||
►
Sign in to add a comment |
|||
Comment 1 by pauljensen@chromium.org
, Jul 24Status: Started (was: Available)