server: Refactoring driver sessions to break direct dependency on system clock to increase testability

Author: barancev

Diff for: java/server/src/org/openqa/selenium/remote/server/Clock.java

@@ -0,0 +1,39 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+package org.openqa.selenium.remote.server;
+
+/**
+ * A simple encapsulation to allowing timing
+ */
+public interface Clock {
+
+  /**
+   * Returns the current time.
+   *
+   * @return The current time.
+   */
+  long now();
+
+  /**
+   * Moves the clock to the future.
+   *
+   * @param durationInMillis Time to pass, in milliseconds.
+   */
+  void pass(long durationInMillis);
+}

Diff for: java/server/src/org/openqa/selenium/remote/server/DefaultDriverSessions.java

@@ -39,6 +39,7 @@ public class DefaultDriverSessions implements DriverSessions {
   private static final Logger LOG = Logger.getLogger(DefaultDriverSessions.class.getName());
 
   private final DriverFactory factory;
+  private final Clock clock;
 
   private final Map<SessionId, Session> sessionIdToDriver =
       new ConcurrentHashMap<SessionId, Session>();
@@ -82,10 +83,16 @@ public DefaultDriverSessions(
     for (Map.Entry<Capabilities, Class<? extends WebDriver>> entry : drivers.entrySet()) {
       registerDriver(entry.getKey(), entry.getValue());
     }
+    this.clock = new SystemClock();
   }
 
   protected DefaultDriverSessions(Platform runningOn, DriverFactory factory) {
+    this(runningOn, factory, new SystemClock());
+  }
+
+  protected DefaultDriverSessions(Platform runningOn, DriverFactory factory, Clock clock) {
     this.factory = factory;
+    this.clock = clock;
     registerDefaults(runningOn);
     registerServiceLoaders(runningOn);
   }
@@ -127,7 +134,7 @@ public void registerDriver(Capabilities capabilities, Class<? extends WebDriver>
 
   public SessionId newSession(Capabilities desiredCapabilities) throws Exception {
     SessionId sessionId = new SessionId(UUID.randomUUID().toString());
-    Session session = DefaultSession.createSession(factory, sessionId, desiredCapabilities);
+    Session session = DefaultSession.createSession(factory, clock, sessionId, desiredCapabilities);
 
     sessionIdToDriver.put(sessionId, session);
 

Diff for: java/server/src/org/openqa/selenium/remote/server/DefaultSession.java

@@ -70,37 +70,44 @@ public class DefaultSession implements Session {
   private final KnownElements knownElements;
   private final ThreadPoolExecutor executor;
   private final Capabilities capabilities; // todo: Investigate memory model implications of map
+  private final Clock clock;
   // elements inside capabilities.
   private volatile String base64EncodedImage;
   private volatile long lastAccess;
   private volatile Thread inUseWithThread = null;
   private TemporaryFilesystem tempFs;
 
   // This method is to avoid constructor escape of partially constructed session object
-  public static Session createSession(DriverFactory factory,
-                                      SessionId sessionId, Capabilities capabilities)
-      throws Exception {
+  public static Session createSession(DriverFactory factory, SessionId sessionId,
+                                      Capabilities capabilities) throws Exception {
+    return createSession(factory, new SystemClock(), sessionId, capabilities);
+  }
+
+  // This method is to avoid constructor escape of partially constructed session object
+  public static Session createSession(DriverFactory factory, Clock clock, SessionId sessionId,
+                                      Capabilities capabilities) throws Exception {
     File tmpDir = new File(System.getProperty("java.io.tmpdir"), sessionId.toString());
     if (!tmpDir.mkdir()) {
       throw new WebDriverException("Cannot create temp directory: " + tmpDir);
     }
     TemporaryFilesystem tempFs = TemporaryFilesystem.getTmpFsBasedOn(tmpDir);
 
-    return new DefaultSession(factory, tempFs, sessionId, capabilities);
+    return new DefaultSession(factory, tempFs, clock, sessionId, capabilities);
   }
 
   @VisibleForTesting
-  public static Session createSession(DriverFactory factory, TemporaryFilesystem tempFs,
+  public static Session createSession(DriverFactory factory, TemporaryFilesystem tempFs, Clock clock,
                                       SessionId sessionId, Capabilities capabilities)
       throws Exception {
-    return new DefaultSession(factory, tempFs, sessionId, capabilities);
+    return new DefaultSession(factory, tempFs, clock, sessionId, capabilities);
   }
 
-  private DefaultSession(final DriverFactory factory, TemporaryFilesystem tempFs,
+  private DefaultSession(final DriverFactory factory, TemporaryFilesystem tempFs, Clock clock,
                          SessionId sessionId, final Capabilities capabilities) throws Exception {
     this.knownElements = new KnownElements();
     this.sessionId = sessionId;
     this.tempFs = tempFs;
+    this.clock = clock;
     final BrowserCreator browserCreator = new BrowserCreator(factory, capabilities);
     final FutureTask<EventFiringWebDriver> webDriverFutureTask =
         new FutureTask<EventFiringWebDriver>(browserCreator);
@@ -141,11 +148,11 @@ private static boolean isQuietModeEnabled(BrowserCreator browserCreator, Capabil
    * Touches the session.
    */
   public void updateLastAccessTime() {
-    lastAccess = System.currentTimeMillis();
+    lastAccess = clock.now();
   }
 
   public boolean isTimedOut(long timeout) {
-    return timeout > 0 && (lastAccess + timeout) < System.currentTimeMillis();
+    return timeout > 0 && (lastAccess + timeout) < clock.now();
   }
 
   public void close() {

Diff for: java/server/src/org/openqa/selenium/remote/server/SessionCleaner.java

@@ -34,11 +34,17 @@ class SessionCleaner extends Thread {   // Thread safety reviewed
   private final long insideBrowserTimeout;
   private final long sleepInterval;
   private final Logger log;
+  private final Clock clock;
   private volatile boolean running = true;
 
   SessionCleaner(DriverSessions driverSessions, Logger log, long clientGoneTimeout, long insideBrowserTimeout) {
+    this(driverSessions, log, new SystemClock(), clientGoneTimeout, insideBrowserTimeout);
+  }
+
+  SessionCleaner(DriverSessions driverSessions, Logger log, Clock clock, long clientGoneTimeout, long insideBrowserTimeout) {
     super("DriverServlet Session Cleaner");
     this.log = log;
+    this.clock = clock;
     this.clientGoneTimeout = clientGoneTimeout;
     this.insideBrowserTimeout = insideBrowserTimeout;
     this.driverSessions = driverSessions;
@@ -60,11 +66,7 @@ class SessionCleaner extends Thread {   // Thread safety reviewed
   public void run() {
     while (running) {
       checkExpiry();
-      try {
-        Thread.sleep(sleepInterval);
-      } catch (InterruptedException e) {
-        log.info("Exiting session cleaner thread");
-      }
+      clock.pass(sleepInterval);
     }
   }
 

Diff for: java/server/src/org/openqa/selenium/remote/server/SystemClock.java

@@ -0,0 +1,33 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+package org.openqa.selenium.remote.server;
+
+public class SystemClock implements Clock {
+
+  public long now() {
+    return System.currentTimeMillis();
+  }
+
+  public void pass(long durationInMillis) {
+    try {
+      Thread.sleep(durationInMillis);
+    } catch (InterruptedException ignore) {
+    }
+  }
+}

Diff for: java/server/src/org/openqa/selenium/remote/server/build.desc

@@ -59,18 +59,20 @@ java_library(name = "server_core",
 java_library(name = "server_very_core",
   srcs = [
     "CapabilitiesComparator.java",
+    "Clock.java",
     "DefaultDriverFactory.java",
     "DefaultDriverProvider.java",
     "DefaultDriverSessions.java",
+    "DefaultSession.java",
     "DriverFactory.java",
     "DriverProvider.java",
     "DriverSessions.java",
-    "KnownElements.java",
     "JsonHttpCommandHandler.java",
+    "KnownElements.java",
     "MimeType.java",
-    "DefaultSession.java",
     "SessionCleaner.java",
     "SnapshotScreenListener.java",
+    "SystemClock.java",
   ],
   deps = [
     ":restish",

Diff for: java/server/test/org/openqa/selenium/remote/server/DefaultSessionTest.java

@@ -39,7 +39,7 @@ public void shouldClearTempFsWhenSessionCloses() throws Exception {
     when(factory.newInstance(any(Capabilities.class))).thenReturn(mock(WebDriver.class));
     final TemporaryFilesystem tempFs = mock(TemporaryFilesystem.class);
 
-    Session session = DefaultSession.createSession(factory, tempFs, null, DesiredCapabilities.firefox());
+    Session session = DefaultSession.createSession(factory, tempFs, new SystemClock(), null, DesiredCapabilities.firefox());
 
     session.close();
     verify(tempFs).deleteTemporaryFiles();

Diff for: java/server/test/org/openqa/selenium/remote/server/FakeClock.java

@@ -0,0 +1,32 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+package org.openqa.selenium.remote.server;
+
+public class FakeClock implements Clock {
+
+  private long now = 0;
+
+  public long now() {
+    return now;
+  }
+
+  public void pass(long durationInMillis) {
+    now += durationInMillis;
+  }
+}

Diff for: java/server/test/org/openqa/selenium/remote/server/SessionCleanerTest.java

@@ -50,7 +50,7 @@ public class SessionCleanerTest {
 
   @Test
   public void testCleanup() throws Exception {
-    DriverSessions defaultDriverSessions = getDriverSessions();
+    DriverSessions defaultDriverSessions = getDriverSessions(new SystemClock());
     defaultDriverSessions.newSession(DesiredCapabilities.firefox());
     defaultDriverSessions.newSession(DesiredCapabilities.firefox());
     assertEquals(2, defaultDriverSessions.getSessions().size());
@@ -110,7 +110,7 @@ public Object call() {
 
   @Test
   public void testCleanupWithThread() throws Exception {
-    DriverSessions defaultDriverSessions = getDriverSessions();
+    DriverSessions defaultDriverSessions = getDriverSessions(new SystemClock());
     defaultDriverSessions.newSession(DesiredCapabilities.firefox());
     defaultDriverSessions.newSession(DesiredCapabilities.firefox());
     assertEquals(2, defaultDriverSessions.getSessions().size());
@@ -144,23 +144,24 @@ void checkExpiry() {
 
   @Test
   public void testCleanupWithSessionExtension() throws Exception {
-    DriverSessions defaultDriverSessions = getDriverSessions();
+    Clock clock = new FakeClock();
+    DriverSessions defaultDriverSessions = getDriverSessions(clock);
     SessionId firstSession = defaultDriverSessions.newSession(DesiredCapabilities.firefox());
     defaultDriverSessions.newSession(DesiredCapabilities.firefox());
-    SessionCleaner sessionCleaner = new SessionCleaner(defaultDriverSessions, log, 100, 100);
+    SessionCleaner sessionCleaner = new SessionCleaner(defaultDriverSessions, log, clock, 100, 100);
     defaultDriverSessions.get(firstSession).updateLastAccessTime();
-    waitForAllSessionsToExpire(120);
+    clock.pass(120);
     defaultDriverSessions.get(firstSession).updateLastAccessTime();
     sessionCleaner.checkExpiry();
     assertEquals(1, defaultDriverSessions.getSessions().size());
-    waitForAllSessionsToExpire(120);
+    clock.pass(120);
     sessionCleaner.checkExpiry();
     assertEquals(0, defaultDriverSessions.getSessions().size());
   }
 
-  private DriverSessions getDriverSessions() {
+  private DriverSessions getDriverSessions(Clock clock) {
     DriverFactory factory = mock(DriverFactory.class);
     when(factory.newInstance(any(Capabilities.class))).thenReturn(mock(WebDriver.class));
-    return new DefaultDriverSessions(Platform.LINUX, factory);
+    return new DefaultDriverSessions(Platform.LINUX, factory, clock);
   }
 }

Diff for: java/server/test/org/openqa/selenium/remote/server/handler/UploadFileTest.java

@@ -42,6 +42,7 @@
 import org.openqa.selenium.remote.server.DefaultSession;
 import org.openqa.selenium.remote.server.DriverFactory;
 import org.openqa.selenium.remote.server.Session;
+import org.openqa.selenium.remote.server.SystemClock;
 
 import java.io.File;
 import java.io.IOException;
@@ -72,7 +73,7 @@ public void cleanUp() {
 
   @Test
   public void shouldWriteABase64EncodedZippedFileToDiskAndKeepName() throws Exception {
-    Session session = DefaultSession.createSession(driverFactory, tempFs, sessionId, DesiredCapabilities.firefox());
+    Session session = DefaultSession.createSession(driverFactory, tempFs, new SystemClock(), sessionId, DesiredCapabilities.firefox());
 
     File tempFile = touch(null, "foo");
     String encoded = new Zip().zipFile(tempFile.getParentFile(), tempFile);
@@ -88,7 +89,7 @@ public void shouldWriteABase64EncodedZippedFileToDiskAndKeepName() throws Except
 
   @Test
   public void shouldThrowAnExceptionIfMoreThanOneFileIsSent() throws Exception {
-    Session session = DefaultSession.createSession(driverFactory, tempFs, sessionId, DesiredCapabilities.firefox());
+    Session session = DefaultSession.createSession(driverFactory, tempFs, new SystemClock(), sessionId, DesiredCapabilities.firefox());
     File baseDir = Files.createTempDir();
 
     touch(baseDir, "example");