From 29ab48bf0dfbac1b249f2233de08fa948bad11d8 Mon Sep 17 00:00:00 2001
From: jwilson <jwilson@squareup.com>
Date: Sat, 28 Sep 2013 15:43:31 -0400
Subject: [PATCH] Fix a race condition in Connection.isExpired().

This was causing some SPDY connections to be evicted early,
and preventing connection reuse.

The spdyConnectionReuse test becomes flaky with the
fix that makes MockWebServer use different sequenceNumbers
for different requests on the same socket. With the fix
the test is no longer flaky. The old test wasn't good enough
to detect what it wanted to detect!

This race has existed in the code since we added more
aggressive time-based expiration in January 2013. In my
basic tests the race impacted ~20% of connections on a
desktop VM. It may have been more on mobile.
---
 .../com/squareup/okhttp/mockwebserver/MockWebServer.java | 3 ++-
 .../squareup/okhttp/internal/spdy/SpdyConnection.java    | 9 ++++++---
 okhttp/src/main/java/com/squareup/okhttp/Connection.java | 2 +-
 .../squareup/okhttp/internal/http/HttpOverSpdyTest.java  | 2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/mockwebserver/src/main/java/com/squareup/okhttp/mockwebserver/MockWebServer.java b/mockwebserver/src/main/java/com/squareup/okhttp/mockwebserver/MockWebServer.java
index 235c8b07e..d036d53c5 100644
--- a/mockwebserver/src/main/java/com/squareup/okhttp/mockwebserver/MockWebServer.java
+++ b/mockwebserver/src/main/java/com/squareup/okhttp/mockwebserver/MockWebServer.java
@@ -633,6 +633,7 @@ public final class MockWebServer {
   private class SpdySocketHandler implements IncomingStreamHandler {
     private final Socket socket;
     private final Transport transport;
+    private final AtomicInteger sequenceNumber = new AtomicInteger();
 
     private SpdySocketHandler(Socket socket, Transport transport) {
       this.socket = socket;
@@ -684,7 +685,7 @@ public final class MockWebServer {
       String requestLine = method + ' ' + path + ' ' + version;
       List<Integer> chunkSizes = Collections.emptyList(); // No chunked encoding for SPDY.
       return new RecordedRequest(requestLine, httpHeaders, chunkSizes, bodyOut.size(),
-          bodyOut.toByteArray(), 0, socket);
+          bodyOut.toByteArray(), sequenceNumber.getAndIncrement(), socket);
     }
 
     private void writeResponse(SpdyStream stream, MockResponse response) throws IOException {
diff --git a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java
index f6c553337..b19bd44c0 100644
--- a/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java
+++ b/okhttp-protocols/src/main/java/com/squareup/okhttp/internal/spdy/SpdyConnection.java
@@ -122,15 +122,18 @@ public final class SpdyConnection implements Closeable {
   }
 
   private synchronized void setIdle(boolean value) {
-    idleStartTimeNs = value ? System.nanoTime() : 0L;
+    idleStartTimeNs = value ? System.nanoTime() : Long.MAX_VALUE;
   }
 
   /** Returns true if this connection is idle. */
   public synchronized boolean isIdle() {
-    return idleStartTimeNs != 0L;
+    return idleStartTimeNs != Long.MAX_VALUE;
   }
 
-  /** Returns the time in ns when this connection became idle or 0L if connection is not idle. */
+  /**
+   * Returns the time in ns when this connection became idle or Long.MAX_VALUE
+   * if connection is not idle.
+   */
   public synchronized long getIdleStartTimeNs() {
     return idleStartTimeNs;
   }
diff --git a/okhttp/src/main/java/com/squareup/okhttp/Connection.java b/okhttp/src/main/java/com/squareup/okhttp/Connection.java
index 6be565670..a6b798da9 100644
--- a/okhttp/src/main/java/com/squareup/okhttp/Connection.java
+++ b/okhttp/src/main/java/com/squareup/okhttp/Connection.java
@@ -244,7 +244,7 @@ public final class Connection implements Closeable {
    * {@code keepAliveDurationNs}.
    */
   public boolean isExpired(long keepAliveDurationNs) {
-    return isIdle() && System.nanoTime() - getIdleStartTimeNs() > keepAliveDurationNs;
+    return getIdleStartTimeNs() < System.nanoTime() - keepAliveDurationNs;
   }
 
   /**
diff --git a/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java b/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java
index ca240f131..7720c5b61 100644
--- a/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java
+++ b/okhttp/src/test/java/com/squareup/okhttp/internal/http/HttpOverSpdyTest.java
@@ -142,7 +142,7 @@ public final class HttpOverSpdyTest {
     assertEquals("DEF", readAscii(connection1.getInputStream(), 3));
     assertEquals("JKL", readAscii(connection2.getInputStream(), 3));
     assertEquals(0, server.takeRequest().getSequenceNumber());
-    assertEquals(0, server.takeRequest().getSequenceNumber());
+    assertEquals(1, server.takeRequest().getSequenceNumber());
   }
 
   @Test public void gzippedResponseBody() throws Exception {