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

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

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



##########
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:
       No. The old code did
   ```
       public static Boolean checkFileExists2(Path path, LinkOption... options) {
           if (Files.exists(path, options)) {
               return Boolean.TRUE;
           } else if (Files.notExists(path, options)) {
               return Boolean.FALSE;
           } else {
               return null;
           }
       }
   ```
   Now let's inline the `exists()` and `notExists()` calls:
   ```
       public static Boolean checkFileExists3(Path path, LinkOption... options) {
           try {
               if (followLinks(options)) {
                   path.getFileSystem().provider().checkAccess(path);
               } else {
                   // attempt to read attributes without following links
                   Files.readAttributes(path, BasicFileAttributes.class,
                                  LinkOption.NOFOLLOW_LINKS);
               }
               // file exists
               return Boolean.TRUE;
           } catch (IOException x) {
               // does not exist or unable to determine if file exists
               // return false;
               // Let's do the notExists check
           }
           try {
               if (followLinks(options)) {
                   path.getFileSystem().provider().checkAccess(path);
               } else {
                   // attempt to read attributes without following links
                   Files.readAttributes(path, BasicFileAttributes.class,
                                  LinkOption.NOFOLLOW_LINKS);
               }
               // file exists
               // return false;
               // In this case we returned TRUE above already
           } catch (NoSuchFileException x) {
               // file confirmed not to exist
               // return true;
               return Boolean.FALSE;
           } catch (IOException x) {
               // return false;
               // We cannot determine it
           }
           return null;
       }
   ```
   As one can see, there are exactly three possibilities:
   
   1. checkAccess/readAttributes doesn't throw an exception: file exists
   2. NoSuchFileException is thrown: file doesn't exist
   3. Some other IOException is thrown: indeterminate.
   
   Unix domain sockets or not, and FileNotFoundException or not -- the old code did exactly what the new code does, but called checkAccess/readAttributes up to two times, while the new code does so only and exactly once.
   
   Moreover, with attribute caching the second call to readAttributes would be no-op anyway. (Which is not a problem, since if it should fail, it should have failed the first time already.)




-- 
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