Skip to content

Commit

Permalink
Improve handling concurrent workspace changes when storing file (#103)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@HeikoKlare
HeikoKlare committed Jun 27, 2023
1 parent b85b3cd commit 41932f2
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 25 deletions.
86 changes: 67 additions & 19 deletions ...se.core.resources/src/org/eclipse/core/internal/localstore/FileSystemResourceManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,62 @@
*******************************************************************************/
package org.eclipse.core.internal.localstore;

import java.io.*;
import java.io.BufferedInputStream;
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.URI;
import java.util.*;
import org.eclipse.core.filesystem.*;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import org.eclipse.core.filesystem.EFS;
import org.eclipse.core.filesystem.IFileInfo;
import org.eclipse.core.filesystem.IFileStore;
import org.eclipse.core.filesystem.IFileTree;
import org.eclipse.core.filesystem.URIUtil;
import org.eclipse.core.internal.refresh.RefreshManager;
import org.eclipse.core.internal.resources.*;
import org.eclipse.core.internal.resources.File;
import org.eclipse.core.internal.utils.*;
import org.eclipse.core.resources.*;
import org.eclipse.core.runtime.*;
import org.eclipse.core.internal.resources.Folder;
import org.eclipse.core.internal.resources.ICoreConstants;
import org.eclipse.core.internal.resources.IManager;
import org.eclipse.core.internal.resources.LinkDescription;
import org.eclipse.core.internal.resources.ModelObjectWriter;
import org.eclipse.core.internal.resources.Project;
import org.eclipse.core.internal.resources.ProjectDescription;
import org.eclipse.core.internal.resources.ProjectDescriptionReader;
import org.eclipse.core.internal.resources.Resource;
import org.eclipse.core.internal.resources.ResourceException;
import org.eclipse.core.internal.resources.ResourceInfo;
import org.eclipse.core.internal.resources.Workspace;
import org.eclipse.core.internal.resources.WorkspaceDescription;
import org.eclipse.core.internal.utils.BitMask;
import org.eclipse.core.internal.utils.FileUtil;
import org.eclipse.core.internal.utils.Messages;
import org.eclipse.core.internal.utils.Policy;
import org.eclipse.core.resources.IContainer;
import org.eclipse.core.resources.IFile;
import org.eclipse.core.resources.IFolder;
import org.eclipse.core.resources.IPathVariableManager;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IProjectDescription;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.resources.IResourceStatus;
import org.eclipse.core.resources.IWorkspaceRoot;
import org.eclipse.core.resources.ResourceAttributes;
import org.eclipse.core.resources.ResourcesPlugin;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.IPath;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.MultiStatus;
import org.eclipse.core.runtime.OperationCanceledException;
import org.eclipse.core.runtime.Path;
import org.eclipse.core.runtime.Platform;
import org.eclipse.core.runtime.Preferences;
import org.eclipse.core.runtime.Preferences.PropertyChangeEvent;
import org.eclipse.core.runtime.SubMonitor;
import org.eclipse.osgi.util.NLS;
import org.xml.sax.InputSource;

Expand Down Expand Up @@ -1099,15 +1143,16 @@ 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 {
SubMonitor subMonitor = SubMonitor.convert(monitor, 4);
try {
assertExists(target);
IFileStore store = getStore(target);
if (fileInfo.getAttribute(EFS.ATTRIBUTE_READ_ONLY)) {
String message = NLS.bind(Messages.localstore_couldNotWriteReadOnly, target.getFullPath());
Expand All @@ -1122,10 +1167,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);
Expand Down Expand Up @@ -1172,6 +1215,7 @@ public void write(IFile target, InputStream content, IFileInfo fileInfo, int upd
subMonitor.split(1);
}
int options = append ? EFS.APPEND : EFS.NONE;
assertExists(target);
try (OutputStream out = store.openOutputStream(options, subMonitor.split(1))) {
if (restoreHiddenAttribute) {
fileInfo.setAttribute(EFS.ATTRIBUTE_HIDDEN, true);
Expand All @@ -1186,11 +1230,8 @@ 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();
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);
Expand All @@ -1200,6 +1241,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.
Expand Down
52 changes: 46 additions & 6 deletions ...sources/src/org/eclipse/core/tests/internal/localstore/FileSystemResourceManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,26 @@
import org.eclipse.core.filesystem.IFileInfo;
import org.eclipse.core.filesystem.IFileStore;
import org.eclipse.core.internal.localstore.FileSystemResourceManager;
import org.eclipse.core.internal.resources.*;
import org.eclipse.core.resources.*;
import org.eclipse.core.runtime.*;
import org.eclipse.core.internal.resources.File;
import org.eclipse.core.internal.resources.ICoreConstants;
import org.eclipse.core.internal.resources.Project;
import org.eclipse.core.internal.resources.Resource;
import org.eclipse.core.internal.resources.Workspace;
import org.eclipse.core.resources.IFile;
import org.eclipse.core.resources.IFolder;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IProjectDescription;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.resources.IWorkspace;
import org.eclipse.core.resources.IWorkspaceRunnable;
import org.eclipse.core.resources.ResourcesPlugin;
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.IPath;
import org.eclipse.core.runtime.IProgressMonitor;
import org.eclipse.core.runtime.IStatus;
import org.eclipse.core.runtime.NullProgressMonitor;
import org.eclipse.core.runtime.Path;
import org.eclipse.core.runtime.Status;
import org.eclipse.core.tests.internal.filesystem.bug440110.Bug440110FileSystem;
import org.junit.Test;

Expand Down Expand Up @@ -282,15 +299,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");
Expand Down Expand Up @@ -390,7 +430,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
Expand Down

0 comments on commit 41932f2

Please sign in to comment.