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

["Thomas Wolf (Jira)" <ji...@apache.org>]

Thomas Wolf created SSHD-1220:
---------------------------------

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


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