[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths

[GitBox <gi...@apache.org>]

lgoldstein commented on a change in pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#discussion_r739611294



##########
File path: sshd-common/src/main/java/org/apache/sshd/common/util/io/IoUtils.java
##########
@@ -372,11 +374,23 @@ public static String getFileOwner(Path path, LinkOption... options) throws IOExc
      *                 explained above
      */
     public static Boolean checkFileExists(Path path, LinkOption... options) {
-        if (Files.exists(path, options)) {
+        boolean followLinks = true;
+        for (LinkOption opt : options) {
+            if (opt == LinkOption.NOFOLLOW_LINKS) {
+                followLinks = false;
+                break;
+            }
+        }
+        try {
+            if (followLinks) {
+                path.getFileSystem().provider().checkAccess(path);
+            } else {
+                Files.readAttributes(path, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS);
+            }
             return Boolean.TRUE;
-        } else if (Files.notExists(path, options)) {
+        } catch (NoSuchFileException e) {

Review comment:
       I am not sure these 2 lines are equivalent - it  assumes that the implementation throws `NoSuchFileException` but this might not be the case. It **should** be, but we know that in reality there are some who do not. Furthermore, strange as it may seem, **both** `File.exists` and `File.notExists`  might return *false* (for "special" files - e.g., Unix domain sockets) - this is why return `Boolean` *null* to indicate such a file. Perhaps a better way would be:
   
   ```java
   try {
       ... check exists ...
   } catch (FileNotFoundException e) {
       return Boolean.FALSE;
   }
   
   return File.notExists(path) ? Boolean.FALSE : null;
   ```

##########
File path: sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpPath.java
##########
@@ -25,12 +25,80 @@
 import java.util.List;
 
 import org.apache.sshd.common.file.util.BasePath;
+import org.apache.sshd.sftp.client.SftpClient;
 
 public class SftpPath extends BasePath<SftpPath, SftpFileSystem> {
+
+    private SftpClient.Attributes attributes;
+
+    private int cachingLevel;
+
     public SftpPath(SftpFileSystem fileSystem, String root, List<String> names) {
         super(fileSystem, root, names);
     }
 
+    /**
+     * {@link SftpPath} instances can cache SFTP {@link SftpClient.Attributes}. Caching can be enabled by passing
+     * {@code true}. If the {@link SftpPath} instance is already caching attributes, a counter is increased only. To
+     * disable caching, pass {@code false}, which decreases the counter. The cache is cleared when the counter reaches
+     * zero again.
+     * <p>
+     * Each call of {@code cacheAttributes(true)} must be matched by a call to {@code cacheAttributes(false)}. Such call
+     * pairs can be nested; caching is enabled for the duration of the outermost such pair. The outermost call passing
+     * {@code true} clears any possibly already cached attributes so that the next attempt to read remote attributes
+     * will fetch them anew.
+     *
+     * @param doCache whether to start caching (increasing the cache level) or to stop caching (decreasing the cache
+     *                level)
+     */
+    protected void cacheAttributes(boolean doCache) {
+        if (doCache) {
+            // Start caching. Clear possibly already cached data
+            if (cachingLevel == 0) {
+                attributes = null;
+            }
+            cachingLevel++;
+        } else if (!doCache) {
+            // Stop caching
+            if (cachingLevel > 0) {
+                cachingLevel--;
+                if (cachingLevel == 0) {
+                    attributes = null;
+                }
+            }
+        }
+    }
+
+    /**
+     * Sets the cached attributes to the argument if this {@link SftpPath} instance is currently caching attributes.
+     * Otherwise a no-op.
+     *
+     * @param attributes the {@link SftpClient.Attributes} to cache
+     */
+    protected void cacheAttributes(SftpClient.Attributes attributes) {
+        if (cachingLevel > 0) {
+            setAttributes(attributes);
+        }
+    }
+
+    /**
+     * Unconditionally set the cached attributes, whether or not this instance's attribute cache is enabled.
+     *
+     * @param attributes the {@link SftpClient.Attributes} to cache
+     */
+    void setAttributes(SftpClient.Attributes attributes) {

Review comment:
       Recommend making this `protected` so implementors can override it




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org