From 75a0ca7875297b2ad3dcc7e8c82b200316d084d3 Mon Sep 17 00:00:00 2001 From: Simon Stewart Date: Thu, 4 May 2017 18:33:19 +0100 Subject: [PATCH] Ensure new server sessions have the same session ID as the session they represent. --- .../remote/server/DefaultDriverSessions.java | 14 ++-- .../remote/server/DefaultSession.java | 81 +++++++++---------- .../remote/server/DefaultSessionTest.java | 5 +- .../remote/server/handler/UploadFileTest.java | 17 ++-- 4 files changed, 61 insertions(+), 56 deletions(-) diff --git a/java/server/src/org/openqa/selenium/remote/server/DefaultDriverSessions.java b/java/server/src/org/openqa/selenium/remote/server/DefaultDriverSessions.java index b870fca4b2751..0635d7b99f38b 100644 --- a/java/server/src/org/openqa/selenium/remote/server/DefaultDriverSessions.java +++ b/java/server/src/org/openqa/selenium/remote/server/DefaultDriverSessions.java @@ -18,10 +18,12 @@ package org.openqa.selenium.remote.server; import com.google.common.collect.ImmutableList; +import com.google.common.io.Files; import org.openqa.selenium.Capabilities; import org.openqa.selenium.Platform; import org.openqa.selenium.WebDriver; +import org.openqa.selenium.io.TemporaryFilesystem; import org.openqa.selenium.remote.DesiredCapabilities; import org.openqa.selenium.remote.SessionId; @@ -30,7 +32,6 @@ import java.util.Map; import java.util.ServiceLoader; import java.util.Set; -import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Logger; @@ -127,12 +128,15 @@ public void registerDriver(Capabilities capabilities, Class } public SessionId newSession(Capabilities desiredCapabilities) throws Exception { - SessionId sessionId = new SessionId(UUID.randomUUID().toString()); - Session session = DefaultSession.createSession(factory, clock, sessionId, desiredCapabilities); + Session session = DefaultSession.createSession( + factory, + TemporaryFilesystem.getTmpFsBasedOn(Files.createTempDir()), + clock, + desiredCapabilities); - sessionIdToDriver.put(sessionId, session); + sessionIdToDriver.put(session.getSessionId(), session); - return sessionId; + return session.getSessionId(); } public Session get(SessionId sessionId) { diff --git a/java/server/src/org/openqa/selenium/remote/server/DefaultSession.java b/java/server/src/org/openqa/selenium/remote/server/DefaultSession.java index 81c04e1e3ebfd..2e1cfdf30ec49 100644 --- a/java/server/src/org/openqa/selenium/remote/server/DefaultSession.java +++ b/java/server/src/org/openqa/selenium/remote/server/DefaultSession.java @@ -17,8 +17,6 @@ package org.openqa.selenium.remote.server; -import com.google.common.annotations.VisibleForTesting; - import org.openqa.selenium.Capabilities; import org.openqa.selenium.HasCapabilities; import org.openqa.selenium.JavascriptExecutor; @@ -26,7 +24,6 @@ import org.openqa.selenium.Rotatable; import org.openqa.selenium.TakesScreenshot; import org.openqa.selenium.WebDriver; -import org.openqa.selenium.WebDriverException; import org.openqa.selenium.html5.ApplicationCache; import org.openqa.selenium.html5.LocationContext; import org.openqa.selenium.html5.WebStorage; @@ -36,10 +33,11 @@ import org.openqa.selenium.mobile.NetworkConnection; import org.openqa.selenium.remote.CapabilityType; import org.openqa.selenium.remote.DesiredCapabilities; +import org.openqa.selenium.remote.RemoteWebDriver; import org.openqa.selenium.remote.SessionId; import org.openqa.selenium.support.events.EventFiringWebDriver; -import java.io.File; +import java.util.UUID; import java.util.concurrent.Callable; import java.util.concurrent.FutureTask; import java.util.concurrent.LinkedBlockingQueue; @@ -77,35 +75,21 @@ public class DefaultSession implements Session { 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 { - 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, clock, sessionId, capabilities); - } - - @VisibleForTesting - public static Session createSession(DriverFactory factory, TemporaryFilesystem tempFs, Clock clock, - SessionId sessionId, Capabilities capabilities) + public static Session createSession( + DriverFactory factory, + TemporaryFilesystem tempFs, + Clock clock, + Capabilities capabilities) throws Exception { - return new DefaultSession(factory, tempFs, clock, sessionId, capabilities); + return new DefaultSession(factory, tempFs, clock, capabilities); } - private DefaultSession(final DriverFactory factory, TemporaryFilesystem tempFs, Clock clock, - SessionId sessionId, final Capabilities capabilities) throws Exception { + private DefaultSession( + final DriverFactory factory, + TemporaryFilesystem tempFs, + Clock clock, + 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); @@ -113,7 +97,7 @@ private DefaultSession(final DriverFactory factory, TemporaryFilesystem tempFs, new FutureTask<>(browserCreator); executor = new ThreadPoolExecutor(1, 1, 600L, TimeUnit.SECONDS, - new LinkedBlockingQueue()); + new LinkedBlockingQueue<>()); // Ensure that the browser is created on the single thread. EventFiringWebDriver initialDriver = execute(webDriverFutureTask); @@ -125,10 +109,14 @@ private DefaultSession(final DriverFactory factory, TemporaryFilesystem tempFs, this.driver = initialDriver; this.capabilities = browserCreator.getCapabilityDescription(); + this.sessionId = browserCreator.getSessionId(); + updateLastAccessTime(); } - private static boolean isQuietModeEnabled(BrowserCreator browserCreator, Capabilities capabilities) { + private static boolean isQuietModeEnabled( + BrowserCreator browserCreator, + Capabilities capabilities) { if (browserCreator.isAndroid()) { return true; } @@ -165,19 +153,14 @@ public void close() { public X execute(final FutureTask future) throws Exception { -/* if (executor.isShutdown()) { - throw new WebDriverException(sessionId + " is closed for further execution"); - } */ - executor.execute(new Runnable() { - public void run() { - inUseWithThread = Thread.currentThread(); - inUseWithThread.setName("Session " + sessionId + " processing inside browser"); - try { - future.run(); - } finally { - inUseWithThread = null; - Thread.currentThread().setName("Session " + sessionId + " awaiting client"); - } + executor.execute(() -> { + inUseWithThread = Thread.currentThread(); + inUseWithThread.setName("Session " + sessionId + " processing inside browser"); + try { + future.run(); + } finally { + inUseWithThread = null; + Thread.currentThread().setName("Session " + sessionId + " awaiting client"); } }); return future.get(); @@ -211,6 +194,7 @@ private class BrowserCreator implements Callable { private final DriverFactory factory; private final Capabilities capabilities; private volatile Capabilities describedCapabilities; + private volatile SessionId sessionId; private volatile boolean isAndroid = false; BrowserCreator(DriverFactory factory, Capabilities capabilities) { @@ -226,6 +210,11 @@ public EventFiringWebDriver call() throws Exception { isAndroid = actualCapabilities.getPlatform().is(Platform.ANDROID); } describedCapabilities = getDescription(rawDriver, actualCapabilities); + if (rawDriver instanceof RemoteWebDriver) { + sessionId = ((RemoteWebDriver) rawDriver).getSessionId(); + } else { + sessionId = new SessionId(UUID.randomUUID().toString()); + } return new EventFiringWebDriver(rawDriver); } @@ -233,6 +222,10 @@ public Capabilities getCapabilityDescription() { return describedCapabilities; } + public SessionId getSessionId() { + return sessionId; + } + public boolean isAndroid() { return isAndroid; } diff --git a/java/server/test/org/openqa/selenium/remote/server/DefaultSessionTest.java b/java/server/test/org/openqa/selenium/remote/server/DefaultSessionTest.java index 25c472891a220..4a551cea18e30 100644 --- a/java/server/test/org/openqa/selenium/remote/server/DefaultSessionTest.java +++ b/java/server/test/org/openqa/selenium/remote/server/DefaultSessionTest.java @@ -39,7 +39,10 @@ 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, new SystemClock(), null, DesiredCapabilities.firefox()); + Session session = DefaultSession.createSession( + factory, tempFs, + new SystemClock(), + DesiredCapabilities.firefox()); session.close(); verify(tempFs).deleteTemporaryFiles(); diff --git a/java/server/test/org/openqa/selenium/remote/server/handler/UploadFileTest.java b/java/server/test/org/openqa/selenium/remote/server/handler/UploadFileTest.java index 626606ca381c0..48d2f831314d9 100644 --- a/java/server/test/org/openqa/selenium/remote/server/handler/UploadFileTest.java +++ b/java/server/test/org/openqa/selenium/remote/server/handler/UploadFileTest.java @@ -38,7 +38,6 @@ import org.openqa.selenium.io.TemporaryFilesystem; import org.openqa.selenium.io.Zip; import org.openqa.selenium.remote.DesiredCapabilities; -import org.openqa.selenium.remote.SessionId; import org.openqa.selenium.remote.server.DefaultSession; import org.openqa.selenium.remote.server.DriverFactory; import org.openqa.selenium.remote.server.Session; @@ -53,7 +52,6 @@ public class UploadFileTest { private DriverFactory driverFactory; private TemporaryFilesystem tempFs; - private SessionId sessionId; private File tempDir; @Before @@ -61,7 +59,6 @@ public void setUp() { driverFactory = mock(DriverFactory.class); when(driverFactory.newInstance(any(Capabilities.class))).thenReturn(mock(WebDriver.class)); tempDir = Files.createTempDir(); - sessionId = new SessionId("foo"); tempFs = TemporaryFilesystem.getTmpFsBasedOn(tempDir); } @@ -73,7 +70,11 @@ public void cleanUp() { @Test public void shouldWriteABase64EncodedZippedFileToDiskAndKeepName() throws Exception { - Session session = DefaultSession.createSession(driverFactory, tempFs, new SystemClock(), sessionId, DesiredCapabilities.firefox()); + Session session = DefaultSession.createSession( + driverFactory, + tempFs, + new SystemClock(), + DesiredCapabilities.firefox()); File tempFile = touch(null, "foo"); String encoded = Zip.zip(tempFile); @@ -89,7 +90,11 @@ public void shouldWriteABase64EncodedZippedFileToDiskAndKeepName() throws Except @Test public void shouldThrowAnExceptionIfMoreThanOneFileIsSent() throws Exception { - Session session = DefaultSession.createSession(driverFactory, tempFs, new SystemClock(), sessionId, DesiredCapabilities.firefox()); + Session session = DefaultSession.createSession( + driverFactory, + tempFs, + new SystemClock(), + DesiredCapabilities.firefox()); File baseDir = Files.createTempDir(); touch(baseDir, "example"); @@ -97,7 +102,7 @@ public void shouldThrowAnExceptionIfMoreThanOneFileIsSent() throws Exception { String encoded = Zip.zip(baseDir); UploadFile uploadFile = new UploadFile(session); - Map args = ImmutableMap.of("file", (Object) encoded); + Map args = ImmutableMap.of("file", encoded); uploadFile.setJsonParameters(args); try {