[jira] [Work logged] (SSHD-1220) SFTP: too many LSTAT calls

["ASF GitHub Bot (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/SSHD-1220?focusedWorklogId=672161&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-672161 ]

ASF GitHub Bot logged work on SSHD-1220:
----------------------------------------

                Author: ASF GitHub Bot
            Created on: 29/Oct/21 20:10
            Start Date: 29/Oct/21 20:10
    Worklog Time Spent: 10m 
      Work Description: tomaswolf opened a new pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207


   This greatly reduces the number of (L)STAT calls made to get SFTP
   attributes. Also, listing directories in reply to SSH_FXP_OPENDIR
   and SSH_FXP_READDIR can in this way still go through the normal
   `java.io.file.FileSystem` abstraction. There is no need anymore to
   bypass the file system to resolve SSHD-1217.
   
   The attributes cached on the `Path` objects returned from the
   `DirectoryStream` are used only for listing the directory. This
   is a similar optimization as was done in the Java libraries for
   a `FileTreeWalker` operating on an Unix filesystem, which may use
   cached attributes via the internal interface
   `sun.nio.fs.BasicFileAttributesHolder`.
   
   Caching is also enabled selectively in the up-front checks for file
   accessibility, which also caused repeated (L)STAT calls. Rewrite
   `IoUtils.checkFileExists()` to load attributes only once.
   
   The new operations for caching attributes on `SftpPath` instances are
   protected only; they are not intended to be used by client code. For
   access in the SFTP implementation itself, paths are now always created
   as `SftpPathImpl` instances, which inherit from `SftpPath` and publish a
   safe `withAttributeCache()` method. Theoretically client code could still
   access these methods, but then it's clear for everyone that these are
   internal operations.


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


Issue Time Tracking
-------------------

            Worklog Id:     (was: 672161)
    Remaining Estimate: 0h
            Time Spent: 10m

> SFTP: too many LSTAT calls
> --------------------------
>
>                 Key: SSHD-1220
>                 URL: https://issues.apache.org/jira/browse/SSHD-1220
>             Project: MINA SSHD
>          Issue Type: Improvement
>    Affects Versions: 2.7.0
>            Reporter: Thomas Wolf
>            Priority: Major
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> While looking at an alternate solution for SSHD-1217 I noticed that the {{AbstractSftpSubsystemHelper}} makes several LSTAT calls for a single {{FileSystem.readAttributes()}} invocation.
> Basically it makes one LSTAT call per supported attributes view; only to collect the returned items then in one single map anyway.
> This doesn't make any sense. SFTP file attributes are a single, defined structure. If {{AbstractSftpSubsystemHelper.getAttributes()}} needs to collect all these different views ({{BasicFileAttributes}}, {{PosixFileAttributes}}, and so on) on these underlying SFTP attributes, then all these views should build upon the same single {{SftpClient.Attributes}} gotten exactly _once_.
> This could be solved with careful temporary caching of SFTP attributes on the {{SftpPath}} once read while in {{AbstractSftpSubsystemHelper.getAttributes()}} and clearing that cache at the end.
> The problem is more general, though. The code in several places makes up-front checks whether a file exists, is a directory, and so on. This is a questionable pattern anyway, since the result of a {{Files.exists()}} is outdated immediately; one cannot rely on the file being still there on the next access. With an {{SftpPath}}, this {{exists()}} call is an (L)STAT remote call getting the attributes. Now look at {{AbstractSftpSubsystemHelper.resolveFileAttributes}}: here getting the attributes themselves is so guarded, so it makes at least _two_ LSTAT calls.
> This should be improved. A solution might be not checking for existence, isDirectory(), and such up front but translating {{SftpExceptions}} at the {{FileSystem}} boundary into more standard exceptions if possible and then just doing the operations. For instance, {{AbstractSftpSubsystemHelper.resolveFileAttributes}} currently is
> {code:java}
>     protected NavigableMap<String, Object> resolveFileAttributes(
>             Path file, int flags, LinkOption... options)
>             throws IOException {
>         Boolean status = IoUtils.checkFileExists(file, options);
>         if (status == null) {
>             return handleUnknownStatusFileAttributes(file, flags, options);
>         } else if (!status) {
>             throw new NoSuchFileException(file.toString(), file.toString(), "Attributes N/A for target");
>         } else {
>             return getAttributes(file, flags, options);
>         }
>     }
> {code}
> The {{getAttributes()}} call will call {{java.nio.file.Files.getAttributes()}}, which calls {{SftpFileSystemProvider.readAttributes()}}. That function is at the {{FileSystem}} API boundary, and should translate an {{SftpException}} with subcode SH_FX_NO_SUCH_FILE into a {{java.nio.file.NoSuchFileException}}. (And a few others.)
> The {{handleUnknownStatusFileAttributes}} mechanism is invoked if the file neither {{Files.exists()}} nor {{Files.notExists()}}. IMO that whole case would just disappear. There is no need for up-front checking in many cases; here either {{getAttributes()}} succeeds or throws an appropriate exception.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

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