Skip to content

Commit

Permalink
Reduce flakiness and document reasons for flakiness
Browse files Browse the repository at this point in the history
Android has been receiving reports of some tests being flaky
on what are probably lower-spec devices.

This introduces delays into tests where sockets are being
poisoned after the entire response body has been written to
them *and* where there are follow-up requests.

This change also improves the documentation for the problematic
SocketPolicy values.
  • Loading branch information
nfuller committed Jan 23, 2015
1 parent 0a19746 commit 90ae2e5
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,7 @@ private boolean processOneRequest(Socket socket, BufferedSource source, Buffered
writeHttpResponse(socket, sink, response);
}

// See warnings associated with these socket policies in SocketPolicy.
if (response.getSocketPolicy() == SocketPolicy.DISCONNECT_AT_END) {
source.close();
sink.close();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ public enum SocketPolicy {
/**
* Close the socket after the response. This is the default HTTP/1.0
* behavior.
*
* <p>Note: Be careful if this used in a test and it is not the final queued request. The client
* is free to continue as soon as it has received the entire response. If it makes a subsequent
* request the server may not have had time to closed the socket. Add delays in the client to
* improve the chances that the server has closed the socket.
*/
DISCONNECT_AT_END,

Expand Down Expand Up @@ -56,17 +61,27 @@ public enum SocketPolicy {
/**
* Shutdown the socket input after sending the response. For testing bad
* behavior.
*
* <p>Note: Be careful if this used in a test and it is not the final queued request. The client
* is free to continue as soon as it has received the response. If it makes a subsequent request
* the server may not have had time to closed the socket. Add delays in the client to improve
* the chances that the server has closed the socket.
*/
SHUTDOWN_INPUT_AT_END,

/**
* Shutdown the socket output after sending the response. For testing bad
* behavior.
*
* <p>Note: Be careful if this used in a test and it is not the final queued request. The client
* is free to continue as soon as it has received the response. If it makes a subsequent request
* the server may not have had time to closed the socket. Add delays in the client to improve
* the chances that the server has closed the socket.
*/
SHUTDOWN_OUTPUT_AT_END,

/**
* Don't response to the request but keep the socket open. For testing
* Don't respond to the request but keep the socket open. For testing
* read response header timeout issue.
*/
NO_RESPONSE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,11 @@ private void testServerClosesOutput(SocketPolicy socketPolicy) throws Exception
connection1.setReadTimeout(100);
assertContent("This connection won't pool properly", connection1);
assertEquals(0, server.takeRequest().getSequenceNumber());

// Give the server time to enact the socket policy if it's one that could happen after the
// client has received the response.
Thread.sleep(500);

HttpURLConnection connection2 = client.open(server.getUrl("/b"));
connection2.setReadTimeout(100);
assertContent("This comes after a busted connection", connection2);
Expand Down Expand Up @@ -632,6 +637,10 @@ private void doUpload(TransferKind uploadKind, WriteKind writeKind) throws Excep
client.client().setHostnameVerifier(new RecordingHostnameVerifier());

assertContent("abc", client.open(server.getUrl("/")));

// Give the server time to disconnect.
Thread.sleep(500);

assertContent("def", client.open(server.getUrl("/")));

Set<TlsVersion> tlsVersions =
Expand Down Expand Up @@ -1197,6 +1206,9 @@ private void testClientConfiguredGzipContentEncodingAndConnectionReuse(TransferK
// Seed the pool with a bad connection.
assertContent("a", client.open(server.getUrl("/")));

// Give the server time to disconnect.
Thread.sleep(500);

// This connection will need to be recovered. When it is, transparent gzip should still work!
assertContent("b", client.open(server.getUrl("/")));

Expand Down Expand Up @@ -2577,6 +2589,9 @@ private void reusedConnectionFailsWithPost(TransferKind transferKind, int reques

assertContent("A", client.open(server.getUrl("/a")));

// Give the server time to disconnect.
Thread.sleep(500);

// If the request body is larger than OkHttp's replay buffer, the failure may still occur.
byte[] requestBody = new byte[requestSize];
new Random(0).nextBytes(requestBody);
Expand Down

0 comments on commit 90ae2e5

Please sign in to comment.