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

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.

resources/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/internal/localstore/FileSystemResourceManagerTest.java

@@ -282,15 +282,38 @@ 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 */
 		ensureDoesNotExistInWorkspace(project);
 	}
 
 	@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