From e86be51784692bad5274de4c12f60a110158c0d5 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Mon, 27 Mar 2023 17:50:46 +0200 Subject: [PATCH] Improve handling concurrent workspace changes when storing file (#103) The FileSystemResourceManager.write method fails when concurrently modifying the target file in the workspace with different kinds of exceptions depending on the point in time at which the file is removed. This can, e.g., be the case due to a concurrently running workspace refresh, which removes the file while write() is executed. The FileSystemResourceManagerTest.testWriteFile randomly fails because of this behavior. A concurrently running refresh removes the file at any point in time during execution of write(). - Clarify that write() expects existance of target file not only when calling the method but also during execution in its documentation - Make write() check existance of the target file whenever it is accessed to detect concurrent modifications and 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 is concurrently removed --- .../localstore/FileSystemResourceManager.java | 28 ++++++++++-------- .../FileSystemResourceManagerTest.java | 29 +++++++++++++++++-- 2 files changed, 42 insertions(+), 15 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 9100d103870..9f0b78adabc 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 @@ -1099,13 +1099,14 @@ 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 while + * executing this method. 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. */ public void write(IFile target, InputStream content, IFileInfo fileInfo, int updateFlags, boolean append, IProgressMonitor monitor) throws CoreException { + assertExists(target); SubMonitor subMonitor = SubMonitor.convert(monitor, 4); try { IFileStore store = getStore(target); @@ -1122,10 +1123,8 @@ public void write(IFile target, InputStream content, IFileInfo fileInfo, int upd } } else { if (target.isLocal(IResource.DEPTH_ZERO)) { + assertExists(target); 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()) { asyncRefresh(target); @@ -1172,6 +1171,7 @@ public void write(IFile target, InputStream content, IFileInfo fileInfo, int upd subMonitor.split(1); } int options = append ? EFS.APPEND : EFS.NONE; + assertExists(target); OutputStream out = store.openOutputStream(options, subMonitor.split(1)); if (restoreHiddenAttribute) { fileInfo.setAttribute(EFS.ATTRIBUTE_HIDDEN, true); @@ -1182,11 +1182,8 @@ public void write(IFile target, InputStream content, IFileInfo fileInfo, int upd FileUtil.transferStreams(content, out, store.toString(), subMonitor.split(1)); // get the new last modified time and stash in the info lastModified = store.fetchInfo().getLastModified(); + assertExists(target); ResourceInfo info = ((Resource) target).getResourceInfo(false, true); - if (info == null) { - // happens see Bug 571133 - throw new IllegalStateException("No ResourceInfo for: " + target); //$NON-NLS-1$ - } updateLocalSync(info, lastModified); info.incrementContentId(); info.clear(M_CONTENT_CACHE); @@ -1196,6 +1193,13 @@ public void write(IFile target, InputStream content, IFileInfo fileInfo, int upd } } + private void assertExists(IFile target) { + if (!target.exists()) { + String message = NLS.bind(Messages.localstore_fileNotFound, target.getFullPath()); + throw new IllegalStateException(message); + } + } + /** * If force is false, this method fails if there is already a resource in * target's location. 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 68afaa3f462..188f72dfeab 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 @@ -282,7 +282,8 @@ 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); - assertThrows("Should fail writing non existing file", CoreException.class, + waitForRefresh(); // wait for refresh to ensure that file is not present in workspace + assertThrows("Should fail writing non existing file", IllegalStateException.class, () -> write(file, another3, false, null)); /* remove trash */ @@ -290,7 +291,29 @@ public void testWriteFile() throws CoreException { } @Test - public void testWriteFile2() { + + public void testWriteFileConcurrentlyRemoved() 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); + + IProgressMonitor monitorRemovingFileOnWriteProgress = new NullProgressMonitor() { + @Override + public void worked(int work) { + ensureDoesNotExistInWorkspace(file); + } + }; + assertThrows("Should fail writing file that is concurrently removed", IllegalStateException.class, + () -> write(file, getContents(content), false, monitorRemovingFileOnWriteProgress)); + } + + @Test + public void testWriteFileNotInWorkspace() { // Bug 571133 IProject project = projects[0]; IFile file = project.getFile("testWriteFile2"); @@ -390,7 +413,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