From 5fcda77243d2d0da1f45eb6a7b175dabd5e81bef Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Mon, 27 Mar 2023 17:50:46 +0200 Subject: [PATCH 1/2] Check preconditions in FileSystemResourceManager.write #103 The FileSystemResourceManager.write() method fails when the target file has been deleted before the write method has been called. The method defines the existence of the file in the workspace as a precondition but does never check that, which makes the method fail when first accessing information that is not available after the deletion, in particular, its ResourceInfo. This is why the method failed when accessing the ResourceInfo. The FileSystemResourceManagerTest.testWriteFile() randomly fails because of this behavior. A concurrently running workspace refresh removes the file right before the write() method is called. - Make write() check its precondition, i.e., the existence of the target file to deterministically fail with an IllegalStateException - Make testWriteFile() wait for the refresh to be finished before executing write() to have deterministic test behavior - Add additional test case validating proper failure of write() when target file has been deleted before Fixes https://github.com/eclipse-platform/eclipse.platform/issues/103 --- .../localstore/FileSystemResourceManager.java | 31 +++++++++++-------- .../FileSystemResourceManagerTest.java | 24 ++++++++++++-- 2 files changed, 39 insertions(+), 16 deletions(-) diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java index d1e46fcd762..fd21f43c651 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java @@ -1157,12 +1157,19 @@ public void updateLocalSync(ResourceInfo info, long localSyncInfo) { public void write(IFile target, InputStream content, IFileInfo fileInfo, int updateFlags, boolean append, IProgressMonitor monitor) throws CoreException { SubMonitor subMonitor = SubMonitor.convert(monitor, 4); try { + Resource targetResource = (Resource) target; IFileStore store = getStore(target); if (fileInfo.getAttribute(EFS.ATTRIBUTE_READ_ONLY)) { String message = NLS.bind(Messages.localstore_couldNotWriteReadOnly, target.getFullPath()); throw new ResourceException(IResourceStatus.FAILED_WRITE_LOCAL, target.getFullPath(), message, null); } long lastModified = fileInfo.getLastModified(); + ResourceInfo immutableTargetResourceInfo = targetResource.getResourceInfo(true, false); + if (immutableTargetResourceInfo == null) { + // If the resource info is null, the resource does not exist in the workspace. + // This violates the method contract, so throw an according exception. + targetResource.checkExists(targetResource.getFlags(immutableTargetResourceInfo), true); + } if (BitMask.isSet(updateFlags, IResource.FORCE)) { if (append && !target.isLocal(IResource.DEPTH_ZERO) && !fileInfo.exists()) { // force=true, local=false, existsInFileSystem=false @@ -1171,12 +1178,8 @@ public void write(IFile target, InputStream content, IFileInfo fileInfo, int upd } } else { if (target.isLocal(IResource.DEPTH_ZERO)) { - ResourceInfo info = ((Resource) target).getResourceInfo(true, false); - if (info == null) { - throw new IllegalStateException("No ResourceInfo for: " + target); //$NON-NLS-1$ - } // test if timestamp is the same since last synchronization - if (lastModified != info.getLocalSyncInfo()) { + if (lastModified != immutableTargetResourceInfo.getLocalSyncInfo()) { asyncRefresh(target); String message = NLS.bind(Messages.localstore_resourceIsOutOfSync, target.getFullPath()); throw new ResourceException(IResourceStatus.OUT_OF_SYNC_LOCAL, target.getFullPath(), message, null); @@ -1235,15 +1238,17 @@ public void write(IFile target, InputStream content, IFileInfo fileInfo, int upd } // get the new last modified time and stash in the info lastModified = store.fetchInfo().getLastModified(); - ResourceInfo info = ((Resource) target).getResourceInfo(false, true); - if (info == null) { - // happens see Bug 571133 - throw new IllegalStateException("No ResourceInfo for: " + target); //$NON-NLS-1$ + ResourceInfo mutableTargetResourceInfo = targetResource.getResourceInfo(false, true); + if (mutableTargetResourceInfo == null) { + // If the resource info is null, the resource must have been concurrently + // removed from the workspace. This violates the method contract, so throw an + // according exception. + targetResource.checkExists(targetResource.getFlags(mutableTargetResourceInfo), true); } - updateLocalSync(info, lastModified); - info.incrementContentId(); - info.clear(M_CONTENT_CACHE); - workspace.updateModificationStamp(info); + updateLocalSync(mutableTargetResourceInfo, lastModified); + mutableTargetResourceInfo.incrementContentId(); + mutableTargetResourceInfo.clear(M_CONTENT_CACHE); + workspace.updateModificationStamp(mutableTargetResourceInfo); } finally { FileUtil.safeClose(content); } diff --git a/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/localstore/FileSystemResourceManagerTest.java b/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/localstore/FileSystemResourceManagerTest.java index 596c2569534..3fd3024eb3a 100644 --- a/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/localstore/FileSystemResourceManagerTest.java +++ b/resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/localstore/FileSystemResourceManagerTest.java @@ -298,6 +298,7 @@ public void testWriteFile() throws CoreException { /* test the overwrite parameter (false) */ ensureDoesNotExistInFileSystem(file); // FIXME Race Condition with asynchronous workplace refresh see Bug 571133 InputStream another3 = getContents(anotherContent); + waitForRefresh(); // wait for refresh to ensure that file is not present in workspace assertThrows("Should fail writing non existing file", CoreException.class, () -> write(file, another3, false, null)); @@ -305,8 +306,25 @@ public void testWriteFile() throws CoreException { ensureDoesNotExistInWorkspace(project); } + // See https://github.com/eclipse-platform/eclipse.platform/issues/103 @Test - public void testWriteFile2() { + public void testWriteDeletedFile() throws CoreException { + /* initialize common objects */ + IProject project = projects[0]; + IFile file = project.getFile("testWriteFile"); + ensureExistsInWorkspace(file, true); + String content = "original"; + + /* write file for the first time */ + write(file, getContents(content), true, null); + + file.delete(true, null); + assertThrows("Should fail writing file that is already deleted", CoreException.class, + () -> write(file, getContents(content), false, null)); + } + + @Test + public void testWriteFileNotInWorkspace() { // Bug 571133 IProject project = projects[0]; IFile file = project.getFile("testWriteFile2"); @@ -316,7 +334,7 @@ public void testWriteFile2() { /* common contents */ String anotherContent = "and this string should not... well, you know..."; InputStream another = getContents(anotherContent); - assertThrows(IllegalStateException.class, () -> write(file, another, false, null)); + assertThrows(CoreException.class, () -> write(file, another, false, null)); /* remove trash */ ensureDoesNotExistInWorkspace(project); @@ -417,7 +435,7 @@ protected void write(final IFile file, final InputStream contents, final boolean try { IWorkspace workspace = getWorkspace(); assertNotNull("workspace cannot be null", workspace); - workspace.run(new WriteFileContents(file, contents, force, getLocalManager()), null); + workspace.run(new WriteFileContents(file, contents, force, getLocalManager()), monitor); } catch (Throwable t) { // Bug 541493: we see unlikely stack traces reported by JUnit here, log the // exceptions in case JUnit filters stack frames From e40518f6658b332c22fd2e81a0bef568b6269f54 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Thu, 12 Oct 2023 13:20:03 +0200 Subject: [PATCH 2/2] Improve documentation of FileSystemResourceManager.write Document the current behavior of the FileSystemResourceManager.write() method in terms of parameter meanings and conditions for exceptions to be thrown. Contributes to https://github.com/eclipse-platform/eclipse.platform/issues/103 --- .../localstore/FileSystemResourceManager.java | 40 ++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java index fd21f43c651..8bc2c11d14d 100644 --- a/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java +++ b/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java @@ -1148,11 +1148,41 @@ public void updateLocalSync(ResourceInfo info, long localSyncInfo) { } /** - * The target must exist in the workspace. The content InputStream is - * closed even if the method fails. If the force flag is false we only write - * the file if it does not exist or if it is already local and the timestamp - * has NOT changed since last synchronization, otherwise a CoreException - * is thrown. + * The target must exist in the workspace and must remain existing throughout + * the execution of this method. The {@code content} {@link InputStream} is + * closed even if the method fails. If the {@link IResource#FORCE} flag is not + * set in {@code updateFlags}, we only write the file if it does not exist or if + * it is already local and the timestamp has not changed since last + * synchronization, otherwise a {@link CoreException} is thrown. + * + * @param target the file to write to + * @param content a stream with the contents to write to {@code target} + * @param fileInfo the info object for the {@code target} file + * @param updateFlags update flags as defined in {@link IResource} + * @param append whether the {@code content} stream shall be appended to + * the existing contents of {@code target} + * @param monitor the progress monitor to report to + * + * @throws CoreException in any of the following cases: + * + * + * @see IResource#FORCE + * @see IResource#KEEP_HISTORY */ public void write(IFile target, InputStream content, IFileInfo fileInfo, int updateFlags, boolean append, IProgressMonitor monitor) throws CoreException { SubMonitor subMonitor = SubMonitor.convert(monitor, 4);