[
https://issues.apache.org/jira/browse/SSHD-1220?focusedWorklogId=672336&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-672336
]
ASF GitHub Bot logged work on SSHD-1220:
----------------------------------------
Author: ASF GitHub Bot
Created on: 30/Oct/21 11:43
Start Date: 30/Oct/21 11:43
Worklog Time Spent: 10m
Work Description: 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 checkFileExists(Path path, LinkOption... options) {
if (Files.exists(path, options)) {
return Boolean.TRUE;
} else if (Files.notExists(path, options)) {
return Boolean.FALSE;
} else {
return null;
}
}
```
Let's first refactor to not use if-else, since all branches return anyway:
```
public static Boolean checkFileExists1(Path path, LinkOption... options)
{
if (Files.exists(path, options)) {
return Boolean.TRUE;
}
if (Files.notExists(path, options)) {
return Boolean.FALSE;
}
return null;
}
```
Now let's inline the `exists()` and `notExists()` calls:
```
public static Boolean checkFileExists2(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-unsubscr...@mina.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Issue Time Tracking
-------------------
Worklog Id: (was: 672336)
Time Spent: 1h 10m (was: 1h)
> 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
> Assignee: Thomas Wolf
> Priority: Major
> Time Spent: 1h 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-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
