[VFS] Optimize FTP directory existence to use CWD instead of parent LIST#759
Open
ilang wants to merge 2 commits intoapache:masterfrom
Open
[VFS] Optimize FTP directory existence to use CWD instead of parent LIST#759ilang wants to merge 2 commits intoapache:masterfrom
ilang wants to merge 2 commits intoapache:masterfrom
Conversation
…s after connection drop setFTPFile() blindly assumed that root-level directories (where getParent() returns null) always exist by creating a synthetic FTPFile with DIRECTORY_TYPE. This caused exists() to return true even after the FTP connection was lost. Fix: add verifyRootDirectory() which uses CWD "." to verify the directory exists on the server. CWD "." checks the current directory (the logical VFS root) which is correct for both configurations: - userDirIsRoot=true: current dir is the user's login directory - userDirIsRoot=false: current dir is "/" (set by the factory CWD) Using "." rather than "/" avoids a semantic mismatch on non-chroot servers with userDirIsRoot=true, where "/" would go to the actual server root instead of the user's login directory. Add FtpClient.changeDirectory() default method and FTPClientWrapper implementation to support CWD from FtpFileObject.
For directory existence checks, CWD is a lightweight control-channel command with no data transfer, while LIST opens a data channel and transfers the entire directory contents. When CWD succeeds (path is a directory), the expensive parent LIST is skipped entirely. When CWD fails (path is a file, symlink, or non-existent), falls back to the original parent LIST behavior. Add FtpClient.isDirectory() with PWD save/restore in FTPClientWrapper to avoid changing the working directory when probing non-root paths. Rename verifyRootDirectory() to verifyDirectory() as it now serves both root and non-root paths. Fix NPE in getTimestampMillis() for CWD-resolved directories: the synthetic FTPFile has no timestamp, so lazily fetch the real FTPFile via parent LIST when timestamp is first requested.
ilang
force-pushed
the
VFS-cwd-optimization
branch
from
0ec1a6e to
c0cec45
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setFTPFile()now tries CWD before falling back to parent LIST for directory existence checksFtpClient.isDirectory()with PWD save/restore to avoid changing the working directorySymlink note
CWD follows symlinks transparently, so a symlink-to-directory is classified as a plain directory via the CWD fast path. This is acceptable because FTP symlink behavior in VFS is provider-internal —
doGetType()resolves symlinks to their target type, andisSymbolicLink()always returnsfalsefor FTP (the FTP provider does not overridedoIsSymbolicLink()). The embedded MINA FTP server also cannot report symlinks in LIST output (it uses the oldjava.io.FileAPI which follows symlinks transparently).Depends on
This PR depends on #757 (root exists on disconnect fix). Please merge #757 first.
Test plan
FtpCwdOptimizationTestwith 5 tests: