You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by GitBox <gi...@apache.org> on 2021/10/29 20:10:17 UTC
[GitHub] [mina-sshd] tomaswolf opened a new pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf merged pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
tomaswolf merged pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207
--
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
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on a change in pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#discussion_r739644691
##########
File path: sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpPath.java
##########
@@ -25,12 +25,80 @@
import java.util.List;
import org.apache.sshd.common.file.util.BasePath;
+import org.apache.sshd.sftp.client.SftpClient;
public class SftpPath extends BasePath<SftpPath, SftpFileSystem> {
+
+ private SftpClient.Attributes attributes;
+
+ private int cachingLevel;
+
public SftpPath(SftpFileSystem fileSystem, String root, List<String> names) {
super(fileSystem, root, names);
}
+ /**
+ * {@link SftpPath} instances can cache SFTP {@link SftpClient.Attributes}. Caching can be enabled by passing
+ * {@code true}. If the {@link SftpPath} instance is already caching attributes, a counter is increased only. To
+ * disable caching, pass {@code false}, which decreases the counter. The cache is cleared when the counter reaches
+ * zero again.
+ * <p>
+ * Each call of {@code cacheAttributes(true)} must be matched by a call to {@code cacheAttributes(false)}. Such call
+ * pairs can be nested; caching is enabled for the duration of the outermost such pair. The outermost call passing
+ * {@code true} clears any possibly already cached attributes so that the next attempt to read remote attributes
+ * will fetch them anew.
+ *
+ * @param doCache whether to start caching (increasing the cache level) or to stop caching (decreasing the cache
+ * level)
+ */
+ protected void cacheAttributes(boolean doCache) {
+ if (doCache) {
+ // Start caching. Clear possibly already cached data
+ if (cachingLevel == 0) {
+ attributes = null;
+ }
+ cachingLevel++;
+ } else if (!doCache) {
+ // Stop caching
+ if (cachingLevel > 0) {
+ cachingLevel--;
+ if (cachingLevel == 0) {
+ attributes = null;
+ }
+ }
+ }
+ }
+
+ /**
+ * Sets the cached attributes to the argument if this {@link SftpPath} instance is currently caching attributes.
+ * Otherwise a no-op.
+ *
+ * @param attributes the {@link SftpClient.Attributes} to cache
+ */
+ protected void cacheAttributes(SftpClient.Attributes attributes) {
+ if (cachingLevel > 0) {
+ setAttributes(attributes);
+ }
+ }
+
+ /**
+ * Unconditionally set the cached attributes, whether or not this instance's attribute cache is enabled.
+ *
+ * @param attributes the {@link SftpClient.Attributes} to cache
+ */
+ void setAttributes(SftpClient.Attributes attributes) {
Review comment:
Refactored anyway. This is now on `SftpPathImpl`.
--
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
[GitHub] [mina-sshd] lgoldstein commented on pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#issuecomment-955328679
> My proposition instead of a pointless config: either we believe this implementation is correct and then check it in without config, or we believe it isn't correct and then we abandon this idea altogether. And if we can't decide what we believe, then let's also abandon this.
You convinced me - I'm with you on this - let's get rid of the configuration for this feature
--
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
[GitHub] [mina-sshd] tomaswolf commented on pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#issuecomment-955198694
@gnodet , do you have time to give this code a review?
--
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
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by 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 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-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
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by 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
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#discussion_r739611294
##########
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:
I am not sure these 2 lines are equivalent - it assumes that the implementation throws `NoSuchFileException` but this might not be the case. It **should** be, but we know that in reality there are some who do not. Furthermore, strange as it may seem, **both** `File.exists` and `File.notExists` might return *false* (for "special" files - e.g., Unix domain sockets) - this is why return `Boolean` *null* to indicate such a file. Perhaps a better way would be:
```java
try {
... check exists ...
} catch (FileNotFoundException e) {
return Boolean.FALSE;
}
return File.notExists(path) ? Boolean.FALSE : null;
```
##########
File path: sshd-sftp/src/main/java/org/apache/sshd/sftp/client/fs/SftpPath.java
##########
@@ -25,12 +25,80 @@
import java.util.List;
import org.apache.sshd.common.file.util.BasePath;
+import org.apache.sshd.sftp.client.SftpClient;
public class SftpPath extends BasePath<SftpPath, SftpFileSystem> {
+
+ private SftpClient.Attributes attributes;
+
+ private int cachingLevel;
+
public SftpPath(SftpFileSystem fileSystem, String root, List<String> names) {
super(fileSystem, root, names);
}
+ /**
+ * {@link SftpPath} instances can cache SFTP {@link SftpClient.Attributes}. Caching can be enabled by passing
+ * {@code true}. If the {@link SftpPath} instance is already caching attributes, a counter is increased only. To
+ * disable caching, pass {@code false}, which decreases the counter. The cache is cleared when the counter reaches
+ * zero again.
+ * <p>
+ * Each call of {@code cacheAttributes(true)} must be matched by a call to {@code cacheAttributes(false)}. Such call
+ * pairs can be nested; caching is enabled for the duration of the outermost such pair. The outermost call passing
+ * {@code true} clears any possibly already cached attributes so that the next attempt to read remote attributes
+ * will fetch them anew.
+ *
+ * @param doCache whether to start caching (increasing the cache level) or to stop caching (decreasing the cache
+ * level)
+ */
+ protected void cacheAttributes(boolean doCache) {
+ if (doCache) {
+ // Start caching. Clear possibly already cached data
+ if (cachingLevel == 0) {
+ attributes = null;
+ }
+ cachingLevel++;
+ } else if (!doCache) {
+ // Stop caching
+ if (cachingLevel > 0) {
+ cachingLevel--;
+ if (cachingLevel == 0) {
+ attributes = null;
+ }
+ }
+ }
+ }
+
+ /**
+ * Sets the cached attributes to the argument if this {@link SftpPath} instance is currently caching attributes.
+ * Otherwise a no-op.
+ *
+ * @param attributes the {@link SftpClient.Attributes} to cache
+ */
+ protected void cacheAttributes(SftpClient.Attributes attributes) {
+ if (cachingLevel > 0) {
+ setAttributes(attributes);
+ }
+ }
+
+ /**
+ * Unconditionally set the cached attributes, whether or not this instance's attribute cache is enabled.
+ *
+ * @param attributes the {@link SftpClient.Attributes} to cache
+ */
+ void setAttributes(SftpClient.Attributes attributes) {
Review comment:
Recommend making this `protected` so implementors can override it
--
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
[GitHub] [mina-sshd] lgoldstein edited a comment on pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
lgoldstein edited a comment on pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#issuecomment-955152765
The code is certainly more complex to follow... It seems OK, but I may have missed something. Perhaps you should ask Guillaume to take a look at it as well - maybe he can locate something we both missed. In view of this I definitely recommend we make this a configurable behavior (default=enabled)
--
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
[GitHub] [mina-sshd] lgoldstein commented on pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#issuecomment-955152765
The code is certainly more complex to follow... It seems OK, but I may have missed something. Perhaps you should ask Guillaume to take a look at it as well - maybe he can locate something we both missed.
--
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
[GitHub] [mina-sshd] tomaswolf edited a comment on pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
tomaswolf edited a comment on pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#issuecomment-955198554
> The code is certainly more complex to follow... It seems OK, but I may have missed something. Perhaps you should ask Guillaume to take a look at it as well - maybe he can locate something we both missed. In view of this I definitely recommend we make this a configurable behavior (default=enabled)
Well, I think you know how I feel about this... Anyway: added such a config. But IMO it only adds unnecessary complication to the code, and users should never set it to false anyway. It really hurts performance. Try the directory listing test with it set to true or false:
- true: takes some 70ms per run on my machine, makes 3 LSTAT calls
- false: takes >2s per run on my machine, makes 10040 LSTAT calls
The caching of attributes is truly _safe_: the unconditionally cached attributes are used _only_ in directory iteration; just like the cached attributes from `sun.nio.fs.BasicFileAttributesHolder` are used exclusively in `FileTreeWalker` in the Java library. And `withAttributeCaching` is used only for short code blocks that are _known_ to only make repeated calls to `readAttributes`, and they clear the cache at the end.
My proposition instead of a pointless config: either we believe this implementation is correct and then check it in without config, or we believe it isn't correct and then we abandon this idea altogether. And if we can't decide what we believe, then let's also abandon this.
--
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
[GitHub] [mina-sshd] tomaswolf commented on pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#issuecomment-955572789
All right; thanks. Here it is again without config.
--
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
[GitHub] [mina-sshd] tomaswolf commented on pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#issuecomment-955198554
> The code is certainly more complex to follow... It seems OK, but I may have missed something. Perhaps you should ask Guillaume to take a look at it as well - maybe he can locate something we both missed. In view of this I definitely recommend we make this a configurable behavior (default=enabled)
Well, I think you know how I feel about this... Anyway: added such a config. But IMO it only adds unnecessary complication to the code, and users should never set it to false anyway. It really hurts performance. Try the directory listing test with it set to true or false:
- true: takes some 70ms per run on my machine, makes 3 LSTAT calls
- false: takes >2s per run on my machine, makes 10040 LSTAT calls
The caching of attributes is truly _safe_: the unconditionally cached attributes are used _only_ in directory iteration; just like the cached attributes from `sun.nio.fs.BasicFileAttributesHolder` are used exclusively in `FileTreeWalker` in the Java library. And `withAttributeCaching` is used only for short code blocks that are _known_ to only make repeated calls to `readAttributes`, and they clear the cache at the end.
My proposition instead of a pointless config: either we believe this implementation is correct and then check it in without config, or we believe it isn't correct and then we abandon this idea altogether.
--
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
[GitHub] [mina-sshd] tomaswolf merged pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
tomaswolf merged pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207
--
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
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by 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 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;
}
}
```
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-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
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#discussion_r739665726
##########
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:
I trust your judgement on this
--
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
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#discussion_r739855590
##########
File path: docs/sftp.md
##########
@@ -532,7 +530,27 @@ if (directoryPath instanceof SftpPath) {
// Not an SFTP path -- get the directory listing in whatever other way is appropriate.
}
```
-So even if an `SftpFileSystem` fulfills the general contract of a `FileSystem`, a client still has to be aware that
+Alternatively, one can also use the fact that Apache MINA sshd caches the SFTP file attributes on the `SftpPath` objects it
+returns from a `DirectoryStream`:
+
+```java
+public void processSftpDirectory(SftpPath directoryPath, BiConsumer<Path, SftpClient.Attributes> process) throws IOException {
+ try (DirectoryStream<Path> dir = Files.newDirectoryStream(directoryPath)) {
+ for (Path path : dir) {
+ if (path instanceof SftpPath) {
+ SftpClient.Attributes attributes = ((SftpPath) path).getAttributes();
+ process.accept(path, attributes);
+ } else {
+ // A DirectoryStream on a directory given by an SftpPath always returns SftpPath instances as elements.
+ throw new IllegalStateException();
Review comment:
Let's add a message to exception along the lines of "Iterated directory entry=" + path + " is not an SftpPath as expected"
--
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
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on a change in pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207#discussion_r740020319
##########
File path: docs/sftp.md
##########
@@ -532,7 +530,27 @@ if (directoryPath instanceof SftpPath) {
// Not an SFTP path -- get the directory listing in whatever other way is appropriate.
}
```
-So even if an `SftpFileSystem` fulfills the general contract of a `FileSystem`, a client still has to be aware that
+Alternatively, one can also use the fact that Apache MINA sshd caches the SFTP file attributes on the `SftpPath` objects it
+returns from a `DirectoryStream`:
+
+```java
+public void processSftpDirectory(SftpPath directoryPath, BiConsumer<Path, SftpClient.Attributes> process) throws IOException {
+ try (DirectoryStream<Path> dir = Files.newDirectoryStream(directoryPath)) {
+ for (Path path : dir) {
+ if (path instanceof SftpPath) {
+ SftpClient.Attributes attributes = ((SftpPath) path).getAttributes();
+ process.accept(path, attributes);
+ } else {
+ // A DirectoryStream on a directory given by an SftpPath always returns SftpPath instances as elements.
+ throw new IllegalStateException();
Review comment:
Done.
--
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
[GitHub] [mina-sshd] tomaswolf merged pull request #207: [SSHD-1220] Cache SFTP attributes on SftpPaths
Posted by GitBox <gi...@apache.org>.
tomaswolf merged pull request #207:
URL: https://github.com/apache/mina-sshd/pull/207
--
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