[grid] Streamline cleaning up of download/upload folders (#12059)

krmahadevan · diemol · web-flow · commit a3df73cf2248 · 2023-05-19T12:10:38.000+02:00
* [grid] Streamline cleaning up of download/upload folders

The download/upload folders should not be evicted
from cache via a timer. They should instead be 
explicitly invalidated only when a session gets
either evicted or invalidated.

The sesssion removal should cascade cleaning up 
of the caches for download and upload folders as well.

* Formatting files

---------

Co-authored-by: Diego Molina &lt;diemol@users.noreply.github.com&gt;
Co-authored-by: Diego Molina &lt;diemol@gmail.com&gt;
diff --git a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java
@@ -182,42 +182,59 @@ protected LocalNode(
             }
             : healthCheck;
 
+    // Do not clear this cache automatically using a timer.
+    // It will be explicitly cleaned up, as and when "sessionToDownloadsDir" is auto cleaned.
     this.uploadsTempFileSystem =
         CacheBuilder.newBuilder()
-            .expireAfterAccess(sessionTimeout)
-            .ticker(ticker)
             .removalListener(
                 (RemovalListener<SessionId, TemporaryFilesystem>)
-                    notification -> {
-                      TemporaryFilesystem tempFS = notification.getValue();
-                      tempFS.deleteTemporaryFiles();
-                      tempFS.deleteBaseDir();
-                    })
+                    notification ->
+                        Optional.ofNullable(notification.getValue())
+                            .ifPresent(
+                                tempFS -> {
+                                  tempFS.deleteTemporaryFiles();
+                                  tempFS.deleteBaseDir();
+                                }))
             .build();
 
+    // Do not clear this cache automatically using a timer.
+    // It will be explicitly cleaned up, as and when "sessionToDownloadsDir" is auto cleaned.
     this.downloadsTempFileSystem =
         CacheBuilder.newBuilder()
-            .expireAfterAccess(sessionTimeout)
-            .ticker(ticker)
             .removalListener(
                 (RemovalListener<UUID, TemporaryFilesystem>)
-                    notification -> {
-                      TemporaryFilesystem tempFS = notification.getValue();
-                      tempFS.deleteTemporaryFiles();
-                      tempFS.deleteBaseDir();
-                    })
+                    notification ->
+                        Optional.ofNullable(notification.getValue())
+                            .ifPresent(
+                                fs -> {
+                                  fs.deleteTemporaryFiles();
+                                  fs.deleteBaseDir();
+                                }))
             .build();
 
+    // Do not clear this cache automatically using a timer.
+    // It will be explicitly cleaned up, as and when "currentSessions" is auto cleaned.
     this.sessionToDownloadsDir =
         CacheBuilder.newBuilder()
-            .expireAfterAccess(sessionTimeout)
-            .ticker(ticker)
             .removalListener(
                 (RemovalListener<SessionId, UUID>)
                     notification -> {
-                      if (notification.getValue() != null) {
-                        this.downloadsTempFileSystem.invalidate(notification.getValue());
-                      }
+                      Optional.ofNullable(notification.getValue())
+                          .ifPresent(
+                              value -> {
+                                downloadsTempFileSystem.invalidate(value);
+                                LOG.warning(
+                                    "Removing Downloads folder associated with "
+                                        + notification.getKey());
+                              });
+                      Optional.ofNullable(notification.getKey())
+                          .ifPresent(
+                              value -> {
+                                uploadsTempFileSystem.invalidate(value);
+                                LOG.warning(
+                                    "Removing Uploads folder associated with "
+                                        + notification.getKey());
+                              });
                     })
             .build();
 
@@ -294,9 +311,6 @@ private void stopTimedOutSession(RemovalNotification<SessionId, SessionSlot> not
       }
       // Attempt to stop the session
       slot.stop();
-      // Invalidate uploads temp file system
-      this.uploadsTempFileSystem.invalidate(id);
-      // Invalidate downloads temp file system
       this.sessionToDownloadsDir.invalidate(id);
       // Decrement pending sessions if Node is draining
       if (this.isDraining()) {
diff --git a/java/test/org/openqa/selenium/grid/node/NodeTest.java b/java/test/org/openqa/selenium/grid/node/NodeTest.java
@@ -49,8 +49,11 @@
 import java.util.Optional;
 import java.util.UUID;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Consumer;
+import java.util.function.Function;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
@@ -144,7 +147,7 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException {
             .add(caps, new TestSessionFactory((id, c) -> new Handler(c)))
             .maximumConcurrentSessions(2);
     if (isDownloadsTestCase) {
-      builder = builder.enableManagedDownloads(true);
+      builder = builder.enableManagedDownloads(true).sessionTimeout(Duration.ofSeconds(1));
     }
     local = builder.build();
 
@@ -636,6 +639,49 @@ void canListFilesToDownload() throws IOException {
     }
   }
 
+  @Test
+  @DisplayName("DownloadsTestCase")
+  void ensureImmunityToSessionTimeOutsForFileDownloads() throws InterruptedException {
+    Consumer<HttpResponse> DOWNLOAD_RSP_VALIDATOR =
+        response -> {
+          Map<String, Object> map = new Json().toType(string(response), Json.MAP_TYPE);
+          assertThat(map).isNotNull();
+          List<String> files =
+              Optional.ofNullable(map.get("value"))
+                  .map(data -> (Map<String, Object>) data)
+                  .map(data -> (List<String>) data.get("names"))
+                  .orElseThrow(() -> new IllegalStateException("Could not find value attribute"));
+          assertThat(files).isEmpty();
+        };
+
+    Function<Session, HttpResponse> TRIGGER_LIST_FILES =
+        session -> {
+          HttpRequest req =
+              new HttpRequest(GET, String.format("/session/%s/se/files", session.getId()));
+          HttpResponse response = node.execute(req);
+          assertThat(response.getStatus()).isEqualTo(200);
+          return response;
+        };
+
+    Either<WebDriverException, CreateSessionResponse> response =
+        node.newSession(createSessionRequest(caps));
+    assertThatEither(response).isRight();
+    Session session = response.right().getSession();
+    try {
+      HttpResponse rsp = TRIGGER_LIST_FILES.apply(session);
+      // Totally we will try list files 3 times, each with a gap of 900 ms.
+      // The session timeout is defined as 1 second. So we try and ensure that
+      // even after trying to download 3 times, we don't hit any errors.
+      for (int i = 0; i < 3; i++) {
+        DOWNLOAD_RSP_VALIDATOR.accept(rsp);
+        node.isSessionOwner(session.getId()); // Keep session active so that we dont timeout
+        TimeUnit.MILLISECONDS.sleep(700);
+      }
+    } finally {
+      node.stop(session.getId());
+    }
+  }
+
   @Test
   void canDownloadFileThrowsErrorMsgWhenDownloadsDirNotSpecified() {
     Either<WebDriverException, CreateSessionResponse> response =
