Skip to content

Commit

Permalink
HUC regression test: disconnect in the middle of connecting.
Browse files Browse the repository at this point in the history
Prior to commit 084b06b (OkHttp 3.4.0),
a HttpURLConnection.disconnect() in the middle of connecting caused an
infinite loop. HUC does not claim to be thread safe, but concurrent
disconnect should generally be supported (it is supported by HttpEngine)
and this bug occurred even when the disconnect() happened in the same
thread, e.g. through the CookieJar.

The infinite loop prior to that CL occurred because
 - The loop in HttpURLConnectionImpl.java:418 did not check for the
   disconnected state,
 - {StreamAllocation,RetryAndFollowUpInterceptor}.recover() returns
   true for canceled StreamAllocations/calls, but
   StreamAllocation.newStream() immediately fails if already canceled.

This bug was specific to HUC and did not affect the Call API because
RetryAndFollowUpInterceptor's (and, in earlier OkHttp versions,
Call.getResponse()'s) infinite loop did check the canceled case
before proceeding.

The bug existed since at least OkHttp 2.7.5, likely introduced by
commit c358656 (OkHttp 2.7.0).

The new test asserts that a disconnect() while constructing the
cookie headers leads to the new connection being aborted. It would
also be permissible for the connection to succeed (as long as it
doesn't deadlock like it used to), but the stricter test seems
reasonable to ensure that any behavior change is deliberate.
A disconnect() while accessing the Cache FileSystem has the same
effect, but this is not covered by the added test because a
new FileSystem wrapper class that delegates all calls would
create code maintenance overhead.
  • Loading branch information
15characterlimi committed Jan 27, 2017
1 parent 894dbfa commit 92bf655
Showing 1 changed file with 34 additions and 0 deletions.
34 changes: 34 additions & 0 deletions okhttp-tests/src/test/java/okhttp3/URLConnectionTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@
*/
package okhttp3;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.Authenticator;
import java.net.ConnectException;
import java.net.CookieHandler;
import java.net.CookieManager;
import java.net.HttpRetryException;
import java.net.HttpURLConnection;
Expand All @@ -35,6 +38,7 @@
import java.net.URL;
import java.net.URLConnection;
import java.net.UnknownHostException;
import java.nio.charset.Charset;
import java.security.KeyStore;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
Expand All @@ -48,6 +52,7 @@
import java.util.Random;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
import java.util.zip.GZIPInputStream;
import javax.net.ServerSocketFactory;
import javax.net.SocketFactory;
Expand All @@ -66,6 +71,7 @@
import okhttp3.internal.SingleInetAddressDns;
import okhttp3.internal.Util;
import okhttp3.internal.Version;
import okhttp3.internal.huc.OkHttpURLConnection;
import okhttp3.internal.tls.SslClient;
import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.MockWebServer;
Expand All @@ -75,6 +81,7 @@
import okio.BufferedSink;
import okio.GzipSink;
import okio.Okio;

import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
Expand Down Expand Up @@ -1154,6 +1161,33 @@ private void initResponseCache() throws IOException {
in.close();
}

@Test public void disconnectDuringConnect_cookieJar() throws Exception {
final AtomicReference<HttpURLConnection> connectionHolder = new AtomicReference<>();
class DisconnectingCookieJar implements CookieJar {
@Override public void saveFromResponse(HttpUrl url, List<Cookie> cookies) { }
@Override
public List<Cookie> loadForRequest(HttpUrl url) {
connectionHolder.get().disconnect();
return Collections.emptyList();
}
}
OkHttpClient client = new okhttp3.OkHttpClient.Builder()
.cookieJar(new DisconnectingCookieJar())
.build();

URL url = server.url("path that should never be accessed").url();
HttpURLConnection connection = new OkHttpURLConnection(url, client);
connectionHolder.set(connection);
try {
connection.getInputStream();
fail("Connection should not be established");
} catch (IOException expected) {
assertEquals("Canceled", expected.getMessage());
} finally {
connection.disconnect();
}
}

@Test public void disconnectBeforeConnect() throws IOException {
server.enqueue(new MockResponse().setBody("A"));

Expand Down

0 comments on commit 92bf655

Please sign in to comment.