You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "Thomas Wolf (Jira)" <ji...@apache.org> on 2021/10/29 20:24:00 UTC
[jira] [Updated] (SSHD-1220) SFTP: too many LSTAT calls
[ https://issues.apache.org/jira/browse/SSHD-1220?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Thomas Wolf updated SSHD-1220:
------------------------------
Description:
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 also be improved.
was:
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.
> 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 also be improved.
--
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