You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by gg...@apache.org on 2026/04/11 11:58:41 UTC
(commons-vfs) branch master updated: [VFS-862] Fix ON_RESOLVE triggering refresh on internal navigation (#761)
This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-vfs.git
The following commit(s) were added to refs/heads/master by this push:
new ed12c6697 [VFS-862] Fix ON_RESOLVE triggering refresh on internal navigation (#761)
ed12c6697 is described below
commit ed12c66973c3f647e5a7c949423dd684a7629909
Author: Ilan Goldfeld <il...@users.noreply.github.com>
AuthorDate: Sat Apr 11 04:58:36 2026 -0700
[VFS-862] Fix ON_RESOLVE triggering refresh on internal navigation (#761)
* [VFS-862] Fix ON_RESOLVE triggering refresh on internal navigation
CacheStrategy.ON_RESOLVE is intended to refresh files when the user
resolves them via the public API. However, internal navigation methods
(getParent, getRoot, getChildren child resolution, symlink resolution)
also called fileSystem.resolveFile(), triggering ON_RESOLVE refreshes
on files the user never asked to refresh.
This became a severe regression after refresh() was changed to
unconditionally clear FtpFileObject.childMap: each child's getParent()
refreshed the parent, clearing its childMap, forcing a new FTP LIST
command per child. A directory with N files produced ~N LIST commands
instead of 1.
Fix:
- Add resolveFileInternal() that skips the ON_RESOLVE refresh. All
internal navigation call sites use it instead of resolveFile().
- After a fresh directory listing, FTP and SFTP providers propagate
metadata to cached child objects in-place, preserving object identity.
This establishes a clear contract: cached state is used until the user
explicitly calls refresh() or resolves via the public API. Internal
navigation never triggers server operations.
Tests:
- FtpGetChildrenListCommandTest: verifies findFiles() on a directory
with 50 files issues exactly 1 LIST command with ON_RESOLVE.
- SftpGetChildrenListCommandTest: verifies refresh + findFiles()
returns fresh children reflecting filesystem changes.
* [VFS-862] Disable HTTP persistent connections in Jackrabbit 1.x tests
Jackrabbit 1.x bundles Jetty 6.x which does not drain unconsumed
request bodies before sending error responses (e.g. 404) on persistent
connections, violating HTTP/1.1 requirements. This leaves stale request
bytes that corrupt the next request on a reused connection.
The resolveFileInternal change in getParent() removed an extra PROPFIND
request to the parent directory that used to happen between the child's
PROPFIND (404) and the child's PUT. That extra request happened to flush
the stale bytes from the connection. Without it, the server reads the
112-byte PROPFIND XML request body as the PUT file content.
Fix: add Connection: close to the test HttpClient configuration so
each request uses a fresh connection.
Jetty 9.4+ handles this correctly (see jetty#651, jetty#4117, jetty#6168).
Jackrabbit 2 tests pass without changes. No production code modified.
* [VFS-862] Remove SFTP test that passes without the fix
The SFTP test cannot distinguish the fix from the original ON_RESOLVE
behavior because SFTP's doListChildrenResolved() pushes metadata to
each child via setStat() immediately after resolution. The ON_RESOLVE
refresh clears attrs, but setStat() repopulates it right after — same
server traffic either way. The optimization is purely client-side.
The FTP test remains and definitively proves the fix (82 LISTs without
the fix, 1 LIST with).
* Add comment.
---------
Co-authored-by: Gary Gregory <ga...@users.noreply.github.com>
---
.../webdav/test/WebdavProviderTestCase.java | 23 +++-
.../commons/vfs2/provider/AbstractFileObject.java | 23 +++-
.../commons/vfs2/provider/AbstractFileSystem.java | 39 +++++-
.../commons/vfs2/provider/ftp/FtpFileObject.java | 25 +++-
.../commons/vfs2/provider/sftp/SftpFileObject.java | 9 +-
.../ftp/FtpGetChildrenListCommandTest.java | 152 +++++++++++++++++++++
6 files changed, 259 insertions(+), 12 deletions(-)
diff --git a/commons-vfs2-jackrabbit1/src/test/java/org/apache/commons/vfs2/provider/webdav/test/WebdavProviderTestCase.java b/commons-vfs2-jackrabbit1/src/test/java/org/apache/commons/vfs2/provider/webdav/test/WebdavProviderTestCase.java
index 64e4875fb..19854e51c 100644
--- a/commons-vfs2-jackrabbit1/src/test/java/org/apache/commons/vfs2/provider/webdav/test/WebdavProviderTestCase.java
+++ b/commons-vfs2-jackrabbit1/src/test/java/org/apache/commons/vfs2/provider/webdav/test/WebdavProviderTestCase.java
@@ -36,6 +36,9 @@ import javax.jcr.Value;
import static org.junit.jupiter.api.Assertions.assertEquals;
+import java.util.Collections;
+
+import org.apache.commons.httpclient.Header;
import org.apache.commons.io.file.PathUtils;
import org.apache.commons.vfs2.AbstractProviderTestConfig;
import org.apache.commons.vfs2.FileObject;
@@ -284,7 +287,25 @@ public class WebdavProviderTestCase extends AbstractProviderTestConfig {
.getFileSystemConfigBuilder("webdav");
final FileSystemOptions opts = new FileSystemOptions();
builder.setRootURI(opts, uri);
- return manager.resolveFile(uri, opts);
+ final FileObject baseFolder = manager.resolveFile(uri, opts);
+
+ // Disable HTTP persistent connections for the embedded Jackrabbit 1.x
+ // test server. Jackrabbit 1.x bundles Jetty 6.x which does not drain
+ // unconsumed request bodies before sending error responses (e.g. 404),
+ // violating HTTP/1.1 persistent connection requirements. This leaves
+ // stale request bytes on the connection that corrupt subsequent requests.
+ // See https://github.com/jetty/jetty.project/issues/651
+ // See https://github.com/jetty/jetty.project/issues/4117
+ final java.lang.reflect.Method getClient =
+ org.apache.commons.vfs2.provider.http.HttpFileSystem.class
+ .getDeclaredMethod("getClient");
+ getClient.setAccessible(true);
+ ((org.apache.commons.httpclient.HttpClient) getClient.invoke(
+ baseFolder.getFileSystem())).getParams().setParameter(
+ "http.default-headers",
+ Collections.singletonList(new Header("Connection", "close")));
+
+ return baseFolder;
}
@Override
diff --git a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileObject.java b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileObject.java
index e4e5703e8..c4f7f4f3c 100644
--- a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileObject.java
+++ b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileObject.java
@@ -1306,7 +1306,7 @@ public abstract class AbstractFileObject<AFS extends AbstractFileSystem> impleme
if (name == null) {
return null;
}
- parent = fileSystem.resolveFile(name);
+ parent = fileSystem.resolveFileInternal(name);
}
return parent;
}
@@ -1805,7 +1805,26 @@ public abstract class AbstractFileObject<AFS extends AbstractFileSystem> impleme
}
private FileObject resolveFile(final FileName child) throws FileSystemException {
- return fileSystem.resolveFile(child);
+ return resolveFileInternal(child);
+ }
+
+ /**
+ * Resolves a file by name for internal navigation, skipping the
+ * {@link org.apache.commons.vfs2.CacheStrategy#ON_RESOLVE} refresh. Cache policy should only
+ * apply to external API calls, not internal VFS plumbing like
+ * {@link #getParent()}, {@link #getChildren()}, or symlink resolution.
+ * <p>
+ * Subclasses should use this instead of {@code getFileSystem().resolveFile()}
+ * when navigating to related files (parent, children, link targets).
+ * </p>
+ *
+ * @param name The FileName to resolve.
+ * @return The resolved FileObject.
+ * @throws FileSystemException if an error occurs.
+ * @since 2.11.0
+ */
+ protected FileObject resolveFileInternal(final FileName name) throws FileSystemException {
+ return fileSystem.resolveFileInternal(name);
}
/**
diff --git a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileSystem.java b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileSystem.java
index 2273fa350..4f3602bb0 100644
--- a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileSystem.java
+++ b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/AbstractFileSystem.java
@@ -377,7 +377,7 @@ public abstract class AbstractFileSystem extends AbstractVfsComponent implements
*/
@Override
public FileObject getRoot() throws FileSystemException {
- return resolveFile(rootName);
+ return resolveFileInternal(rootName);
}
/**
@@ -525,11 +525,39 @@ public abstract class AbstractFileSystem extends AbstractVfsComponent implements
*/
@Override
public FileObject resolveFile(final FileName name) throws FileSystemException {
- return resolveFile(name, true);
+ return resolveFile(name, true, true);
}
- private synchronized FileObject resolveFile(final FileName name, final boolean useCache)
- throws FileSystemException {
+ /**
+ * Resolves a file by name for internal navigation (e.g. {@code getParent()},
+ * {@code resolveFiles()} in {@code getChildren()}). Skips the
+ * {@link CacheStrategy#ON_RESOLVE} refresh, since cache policy should only
+ * apply to external API calls, not internal VFS plumbing. Without this,
+ * {@code ON_RESOLVE} triggers a refresh cascade where each child's
+ * {@code getParent()} refreshes the parent, clearing its cached state and
+ * causing O(N) redundant operations.
+ *
+ * @param name The FileName to resolve.
+ * @return The resolved FileObject.
+ * @throws FileSystemException if an error occurs.
+ * @since 2.11.0
+ */
+ synchronized FileObject resolveFileInternal(final FileName name) throws FileSystemException {
+ return resolveFile(name, true, false);
+ }
+
+ /**
+ * Resolves a file by name.
+ *
+ * @param name The FileName to resolve.
+ * @param useCache whether to use the file cache.
+ * @param applyRefreshPolicy whether to apply the {@link CacheStrategy#ON_RESOLVE} refresh.
+ * Should be {@code true} for external API calls and {@code false} for internal navigation.
+ * @return The resolved FileObject.
+ * @throws FileSystemException if an error occurs.
+ */
+ private synchronized FileObject resolveFile(final FileName name, final boolean useCache,
+ final boolean applyRefreshPolicy) throws FileSystemException {
if (!rootName.getRootURI().equals(name.getRootURI())) {
throw new FileSystemException("vfs.provider/mismatched-fs-for-name.error", name, rootName,
name.getRootURI());
@@ -561,7 +589,8 @@ public abstract class AbstractFileSystem extends AbstractVfsComponent implements
/*
resync the file information if requested
*/
- if (getFileSystemManager().getCacheStrategy().equals(CacheStrategy.ON_RESOLVE)) {
+ if (applyRefreshPolicy
+ && getFileSystemManager().getCacheStrategy().equals(CacheStrategy.ON_RESOLVE)) {
file.refresh();
}
return file;
diff --git a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ftp/FtpFileObject.java b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ftp/FtpFileObject.java
index d8228963e..babec6c4f 100644
--- a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ftp/FtpFileObject.java
+++ b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/ftp/FtpFileObject.java
@@ -501,8 +501,29 @@ public class FtpFileObject extends AbstractFileObject<FtpFileSystem> {
* has C children, P parents, there will be (C * P) listings made with (C * (P + 1)) refreshes, when there
* should really only be 1 listing and C refreshes.
*/
+ final boolean freshList = childMap == null;
inRefresh.set(true);
- return super.getChildren();
+ final FileObject[] result = super.getChildren();
+ // [VFS-862] Fix ON_RESOLVE triggering refresh on internal navigation.
+ // If a fresh LIST was done, propagate the new metadata to cached
+ // child objects so they reflect the latest directory listing.
+ if (freshList && childMap != null) {
+ synchronized (getFileSystem()) {
+ for (final FileObject child : result) {
+ final FtpFileObject ftpChild = (FtpFileObject) FileObjectUtils
+ .getAbstractFileObject(child);
+ final FTPFile entry = childMap.get(
+ UriParser.decode(child.getName().getBaseName()));
+ if (entry != null) {
+ ftpChild.ftpFile = entry;
+ ftpChild.linkDestination = null;
+ ftpChild.mdtmSet = false;
+ ftpChild.injectType(null);
+ }
+ }
+ }
+ }
+ return result;
} finally {
inRefresh.set(false);
}
@@ -530,7 +551,7 @@ public class FtpFileObject extends AbstractFileObject<FtpFileSystem> {
final FileName parent = getName().getParent();
final FileName relativeTo = parent == null ? getName() : parent;
final FileName linkDestinationName = getFileSystem().getFileSystemManager().resolveName(relativeTo, path);
- linkDestination = getFileSystem().resolveFile(linkDestinationName);
+ linkDestination = resolveFileInternal(linkDestinationName);
}
return linkDestination;
}
diff --git a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileObject.java b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileObject.java
index 678d11258..c0ffc61e7 100644
--- a/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileObject.java
+++ b/commons-vfs2/src/main/java/org/apache/commons/vfs2/provider/sftp/SftpFileObject.java
@@ -346,10 +346,15 @@ public class SftpFileObject extends AbstractFileObject<SftpFileSystem> {
continue;
}
- final FileObject fo = getFileSystem().resolveFile(getFileSystem().getFileSystemManager()
+ final FileObject fo = resolveFileInternal(getFileSystem().getFileSystemManager()
.resolveName(getName(), UriParser.encode(name), NameScope.CHILD));
- ((SftpFileObject) FileObjectUtils.getAbstractFileObject(fo)).setStat(stat.getAttrs());
+ final SftpFileObject sftpChild = (SftpFileObject) FileObjectUtils.getAbstractFileObject(fo);
+ sftpChild.setStat(stat.getAttrs());
+ // Clear cached type so it re-evaluates from the fresh attrs.
+ // Without this, a cached type from a prior listing is returned
+ // without consulting the updated attrs.
+ sftpChild.injectType(null);
children.add(fo);
}
diff --git a/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/ftp/FtpGetChildrenListCommandTest.java b/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/ftp/FtpGetChildrenListCommandTest.java
new file mode 100644
index 000000000..a84d325e1
--- /dev/null
+++ b/commons-vfs2/src/test/java/org/apache/commons/vfs2/provider/ftp/FtpGetChildrenListCommandTest.java
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.vfs2.provider.ftp;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.time.Duration;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.commons.vfs2.CacheStrategy;
+import org.apache.commons.vfs2.FileDepthSelector;
+import org.apache.commons.vfs2.FileObject;
+import org.apache.commons.vfs2.FileSystemOptions;
+import org.apache.commons.vfs2.VfsTestUtils;
+import org.apache.commons.vfs2.impl.DefaultFileSystemManager;
+import org.apache.ftpserver.command.CommandFactoryFactory;
+import org.apache.ftpserver.command.impl.LIST;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests that {@link CacheStrategy#ON_RESOLVE} does not cause redundant FTP LIST
+ * commands due to internal VFS navigation triggering cache refreshes.
+ * <p>
+ * With {@code ON_RESOLVE}, every {@code fileSystem.resolveFile()} call triggers
+ * {@code refresh()} on the returned file. This is correct for external API calls
+ * (the user explicitly resolving a file), but internal navigation methods like
+ * {@code getParent()}, {@code getRoot()}, and child resolution in
+ * {@code getChildren()} also call {@code resolveFile()}, unintentionally refreshing
+ * cached files and clearing their state.
+ * </p>
+ * <p>
+ * This was always wasteful, but became a severe regression after
+ * {@code FtpFileObject.refresh()} was changed to unconditionally clear
+ * {@code childMap} (to fix stale cached state on never-attached files).
+ * That change turned the unnecessary refreshes into O(N) LIST commands:
+ * each child's {@code getParent()} refreshes the parent, clearing its
+ * {@code childMap}, forcing a new LIST for every child.
+ * </p>
+ */
+public class FtpGetChildrenListCommandTest {
+
+ private static final int FILE_COUNT = 50;
+ private static final AtomicInteger listCommandCount = new AtomicInteger();
+ private static final LIST defaultListCommand = new LIST();
+
+ private File[] testFiles;
+
+ @BeforeEach
+ public void setUp() throws Exception {
+ listCommandCount.set(0);
+
+ // Create test files in the FTP home directory.
+ final File testDir = new File(VfsTestUtils.getTestDirectory());
+ testFiles = new File[FILE_COUNT];
+ for (int i = 0; i < FILE_COUNT; i++) {
+ testFiles[i] = new File(testDir, "amplification-test-" + i + ".txt");
+ try (FileWriter w = new FileWriter(testFiles[i])) {
+ w.write("content-" + i);
+ }
+ }
+
+ // Start FTP server with a LIST command wrapper that counts invocations.
+ final CommandFactoryFactory cmdFactory = new CommandFactoryFactory();
+ cmdFactory.addCommand("LIST", (session, context, request) -> {
+ listCommandCount.incrementAndGet();
+ defaultListCommand.execute(session, context, request);
+ });
+ FtpProviderTest.setUpClass(VfsTestUtils.getTestDirectory(), null,
+ cmdFactory.createCommandFactory());
+ }
+
+ @AfterEach
+ public void tearDown() {
+ FtpProviderTest.tearDownClass();
+ if (testFiles != null) {
+ for (final File f : testFiles) {
+ f.delete();
+ }
+ }
+ }
+
+ private static FileSystemOptions createOptions() {
+ final FileSystemOptions options = new FileSystemOptions();
+ final FtpFileSystemConfigBuilder builder = FtpFileSystemConfigBuilder.getInstance();
+ builder.setUserDirIsRoot(options, true);
+ builder.setPassiveMode(options, true);
+ builder.setConnectTimeout(options, Duration.ofSeconds(10));
+ return options;
+ }
+
+ /**
+ * Verifies that {@code findFiles()} on a directory with {@code N} children
+ * issues exactly 1 LIST command. {@code findFiles()} internally calls
+ * {@code getType()} on each child via {@code traverse()}, which is where
+ * the cascade occurs.
+ * <p>
+ * With the bug, each child's {@code getType()} triggers a parent LIST via
+ * the ON_RESOLVE cascade, producing ~N LIST commands.
+ * Without the bug, the parent's {@code childMap} is reused across children,
+ * resulting in exactly 1 LIST.
+ * </p>
+ */
+ @Test
+ public void testGetChildrenIssuesSingleList() throws Exception {
+ try (final DefaultFileSystemManager manager = new DefaultFileSystemManager()) {
+ manager.addProvider("ftp", new FtpFileProvider());
+ manager.setCacheStrategy(CacheStrategy.ON_RESOLVE);
+ manager.init();
+
+ final FileSystemOptions options = createOptions();
+ final FileObject root = manager.resolveFile(
+ FtpProviderTest.getConnectionUri(), options);
+
+ listCommandCount.set(0);
+ root.refresh();
+ final FileObject[] children = root.findFiles(new FileDepthSelector(1, 1));
+ assertNotNull(children, "findFiles should return results");
+ assertTrue(children.length >= FILE_COUNT,
+ "Should find at least " + FILE_COUNT + " files, found: " + children.length);
+
+ final int listCount = listCommandCount.get();
+
+ // With the bug: LIST count ≈ FILE_COUNT (one per child due to cascade)
+ // Without the bug: exactly 1 LIST (the getChildren() directory listing)
+ assertEquals(1, listCount,
+ "Scan should produce exactly 1 LIST command, but produced "
+ + listCount + ". This indicates the ON_RESOLVE childMap "
+ + "amplification bug: each child's getParent() re-resolution "
+ + "refreshes the parent, clearing its childMap and forcing a new LIST.");
+ }
+ }
+}