Skip to content

Commit

Permalink
Handle files left open with immutable workspace mechanics on Windows (#…
Browse files Browse the repository at this point in the history 
…27851)
  • Loading branch information
@bot-gradle
bot-gradle committed Jan 30, 2024
2 parents d4609a0 + c0cd6d6 commit f1b03c0
Show file tree
Hide file tree
Showing 3 changed files with 262 additions and 43 deletions.
160 changes: 124 additions & 36 deletions ...ution/src/main/java/org/gradle/internal/execution/steps/AssignImmutableWorkspaceStep.java
View file
Expand Up @@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSortedMap;
import org.apache.commons.io.FileUtils;
import org.gradle.caching.internal.origin.OriginMetadata;
import org.gradle.internal.execution.ExecutionEngine.Execution;
import org.gradle.internal.execution.ImmutableUnitOfWork;
Expand All @@ -35,20 +36,27 @@
import org.gradle.internal.snapshot.FileSystemLocationSnapshot;
import org.gradle.internal.snapshot.FileSystemSnapshot;
import org.gradle.internal.vfs.FileSystemAccess;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.annotation.CheckReturnValue;
import java.io.File;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.FileSystemException;
import java.nio.file.StandardCopyOption;
import java.time.Duration;
import java.util.Map;
import java.util.function.Supplier;
import java.util.Optional;
import java.util.function.Function;

import static com.google.common.collect.ImmutableListMultimap.toImmutableListMultimap;
import static com.google.common.collect.Maps.immutableEntry;
import static org.gradle.internal.execution.ExecutionEngine.ExecutionOutcome.UP_TO_DATE;

public class AssignImmutableWorkspaceStep<C extends IdentityContext> implements Step<C, WorkspaceResult> {
private static final Logger LOGGER = LoggerFactory.getLogger(AssignImmutableWorkspaceStep.class);

private final Deleter deleter;
private final FileSystemAccess fileSystemAccess;

Expand Down Expand Up @@ -76,37 +84,49 @@ public WorkspaceResult execute(UnitOfWork work, C context) {
String uniqueId = context.getIdentity().getUniqueId();
ImmutableWorkspace workspace = workspaceProvider.getWorkspace(uniqueId);

return returnImmutableWorkspaceOr(work, workspace.getImmutableLocation(),
() -> executeInTemporaryWorkspace(work, context, workspace));
return loadImmutableWorkspaceIfExists(work, workspace.getImmutableLocation())
.orElseGet(() -> executeInTemporaryWorkspace(work, context, workspace));
}

private WorkspaceResult returnImmutableWorkspaceOr(UnitOfWork work, File immutableLocation, Supplier<WorkspaceResult> missingImmutableWorkspaceAction) {
private Optional<WorkspaceResult> loadImmutableWorkspaceIfExists(UnitOfWork work, File immutableLocation) {
FileSystemLocationSnapshot workspaceSnapshot = fileSystemAccess.read(immutableLocation.getAbsolutePath());
switch (workspaceSnapshot.getType()) {
case Directory:
return returnUpToDateImmutableWorkspace(work, immutableLocation);
return Optional.of(loadImmutableWorkspace(work, immutableLocation));
case RegularFile:
throw new IllegalStateException("Immutable workspace is occupied by a file: " + immutableLocation.getAbsolutePath());
throw new IllegalStateException(
"Immutable workspace is occupied by a file: " + immutableLocation.getAbsolutePath() + ". " +
"Deleting the file in question can allow the content to be recreated.");
case Missing:
return missingImmutableWorkspaceAction.get();
return Optional.empty();
default:
throw new AssertionError();
}
}

private WorkspaceResult returnUpToDateImmutableWorkspace(UnitOfWork work, File immutableWorkspace) {
ImmutableSortedMap<String, FileSystemSnapshot> outputSnapshots = outputSnapshotter.snapshotOutputs(work, immutableWorkspace);
private WorkspaceResult loadImmutableWorkspace(UnitOfWork work, File immutableLocation) {
ImmutableSortedMap<String, FileSystemSnapshot> outputSnapshots = outputSnapshotter.snapshotOutputs(work, immutableLocation);

// Verify output hashes
ImmutableListMultimap<String, HashCode> outputHashes = calculateOutputHashes(outputSnapshots);
ImmutableWorkspaceMetadata metadata = workspaceMetadataStore.loadWorkspaceMetadata(immutableWorkspace);
ImmutableWorkspaceMetadata metadata = workspaceMetadataStore.loadWorkspaceMetadata(immutableLocation);
if (!metadata.getOutputPropertyHashes().equals(outputHashes)) {
throw new IllegalStateException("Workspace has been changed: " + immutableWorkspace.getAbsolutePath());
throw new IllegalStateException(
"Immutable workspace contents have been modified: " + immutableLocation.getAbsolutePath() + ". " +
"These workspace directories are not supposed to be modified once they are created. " +
"Deleting the directory in question can allow the content to be recreated.");
}

OriginMetadata originMetadata = metadata.getOriginMetadata();
ExecutionOutputState afterExecutionOutputState = new DefaultExecutionOutputState(true, outputSnapshots, originMetadata, true);
return new WorkspaceResult(CachingResult.shortcutResult(Duration.ZERO, Execution.skipped(UP_TO_DATE, work), afterExecutionOutputState, null, originMetadata), immutableWorkspace);
return new WorkspaceResult(
CachingResult.shortcutResult(
Duration.ZERO,
Execution.skipped(UP_TO_DATE, work),
afterExecutionOutputState,
null,
originMetadata),
immutableLocation);
}

private WorkspaceResult executeInTemporaryWorkspace(UnitOfWork work, C context, ImmutableWorkspace workspace) {
Expand All @@ -131,36 +151,15 @@ private WorkspaceResult executeInTemporaryWorkspace(UnitOfWork work, C context,
ImmutableWorkspaceMetadata metadata = new ImmutableWorkspaceMetadata(executionOutputState.getOriginMetadata(), outputHashes);
workspaceMetadataStore.storeWorkspaceMetadata(temporaryWorkspace, metadata);

File immutableLocation = workspace.getImmutableLocation();
try {
fileSystemAccess.moveAtomically(temporaryWorkspace.getAbsolutePath(), immutableLocation.getAbsolutePath());
} catch (FileSystemException e) {
// `Files.move()` says it would throw DirectoryNotEmptyException, but it's a lie, so this is the best we can catch here
return returnUpToDateResult(work, temporaryWorkspace, immutableLocation);
} catch (IOException e) {
throw new UncheckedIOException("Could not move temporary workspace to immutable cache: " + temporaryWorkspace.getAbsolutePath(), e);
}
return new WorkspaceResult(delegateResult, immutableLocation);
return moveTemporaryWorkspaceToImmutableLocation(
new WorkspaceMoveHandler(work, workspace, temporaryWorkspace, workspace.getImmutableLocation(), delegateResult));
} else {
// TODO Do not try to capture the workspace in case of a failure
// TODO Do not capture a null workspace in case of a failure
return new WorkspaceResult(delegateResult, null);
}
});
}

private WorkspaceResult returnUpToDateResult(UnitOfWork work, File temporaryWorkspace, File immutableLocation) {
WorkspaceResult upToDateResult = returnImmutableWorkspaceOr(work, immutableLocation,
() -> {
throw new IllegalStateException("Immutable workspace gone missing again");
});
try {
deleter.deleteRecursively(temporaryWorkspace);
} catch (IOException e) {
throw new UncheckedIOException("Could not remove temporary workspace: " + temporaryWorkspace.getAbsolutePath(), e);
}
return upToDateResult;
}

private static ImmutableListMultimap<String, HashCode> calculateOutputHashes(ImmutableSortedMap<String, FileSystemSnapshot> outputSnapshots) {
return outputSnapshots.entrySet().stream()
.flatMap(entry ->
Expand All @@ -171,4 +170,93 @@ private static ImmutableListMultimap<String, HashCode> calculateOutputHashes(Imm
Map.Entry::getValue
));
}

private WorkspaceResult moveTemporaryWorkspaceToImmutableLocation(WorkspaceMoveHandler move) {
return move.executeMoveOr(moveFailedException -> {
// On Windows, files left open by the executed work can legitimately prevent an atomic move of the temporary directory
// In this case we'll try to make a copy of the temporary workspace to another temporary workspace, and move that to
// the immutable location and then delete the original temporary workspace (if we can).
LOGGER.debug("Could not move temporary workspace ({}) to immutable location ({}), attempting copy-then-move",
move.temporaryWorkspace.getAbsolutePath(), move.immutableLocation.getAbsolutePath(), moveFailedException);
return move.workspace.withTemporaryWorkspace(secondaryTemporaryWorkspace -> {
WorkspaceResult result = move
.duplicateTemporaryWorkspaceTo(secondaryTemporaryWorkspace)
.executeMoveOrThrow();
move.removeTemporaryWorkspace();
return result;
});
});
}

private class WorkspaceMoveHandler {
private final UnitOfWork work;
private final ImmutableWorkspace workspace;
private final File temporaryWorkspace;
private final File immutableLocation;
private final CachingResult delegateResult;

public WorkspaceMoveHandler(UnitOfWork work, ImmutableWorkspace workspace, File temporaryWorkspace, File immutableLocation, CachingResult delegateResult) {
this.work = work;
this.workspace = workspace;
this.temporaryWorkspace = temporaryWorkspace;
this.immutableLocation = immutableLocation;
this.delegateResult = delegateResult;
}

public WorkspaceResult executeMoveOr(Function<FileSystemException, WorkspaceResult> failedMoveHandler) {
try {
fileSystemAccess.moveAtomically(temporaryWorkspace.getAbsolutePath(), immutableLocation.getAbsolutePath());
return new WorkspaceResult(delegateResult, immutableLocation);
} catch (FileSystemException moveWorkspaceException) {
// `Files.move()` says it would throw DirectoryNotEmptyException, but it's a lie, so this is the best we can catch here
if (immutableLocation.isDirectory()) {
LOGGER.debug("Could not move temporary workspace ({}) to immutable location ({}), assuming it was moved in place concurrently",
temporaryWorkspace.getAbsolutePath(), immutableLocation.getAbsolutePath(), moveWorkspaceException);
WorkspaceResult existingImmutableResult = loadImmutableWorkspace(work, immutableLocation);
removeTemporaryWorkspace();
return existingImmutableResult;
} else {
return failedMoveHandler.apply(moveWorkspaceException);
}
} catch (IOException e) {
throw unableToMoveBecause(e);
}
}

public WorkspaceResult executeMoveOrThrow() {
return executeMoveOr(moveFailedException -> {
throw unableToMoveBecause(moveFailedException);
});
}

public WorkspaceMoveHandler duplicateTemporaryWorkspaceTo(File duplicateTemporaryWorkspace) {
try {
FileUtils.copyDirectory(temporaryWorkspace, duplicateTemporaryWorkspace, file -> true, true, StandardCopyOption.COPY_ATTRIBUTES);
} catch (IOException duplicateCopyException) {
throw new UncheckedIOException(
String.format("Could not make copy of temporary workspace (%s) to (%s)",
temporaryWorkspace.getAbsolutePath(), duplicateTemporaryWorkspace.getAbsolutePath()), duplicateCopyException);
}
return new WorkspaceMoveHandler(work, workspace, duplicateTemporaryWorkspace, immutableLocation, delegateResult);
}

private void removeTemporaryWorkspace() {
try {
deleter.deleteRecursively(temporaryWorkspace);
} catch (IOException removeTempException) {
// On Windows it is possible that workspaces with open files cannot be deleted
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Could not remove temporary workspace: {}", temporaryWorkspace.getAbsolutePath(), removeTempException);
} else {
LOGGER.info("Could not remove temporary workspace: {}: {}", temporaryWorkspace.getAbsolutePath(), removeTempException.getMessage());
}
}
}

@CheckReturnValue
private UncheckedIOException unableToMoveBecause(IOException cause) {
return new UncheckedIOException(String.format("Could not move temporary workspace (%s) to immutable location (%s)",
temporaryWorkspace.getAbsolutePath(), immutableLocation.getAbsolutePath()), cause);
}
}
}
75 changes: 68 additions & 7 deletions ...c/test/groovy/org/gradle/internal/execution/steps/AssignImmutableWorkspaceStepTest.groovy
View file
Expand Up @@ -37,7 +37,9 @@ import org.gradle.internal.snapshot.FileSystemLocationSnapshot
import org.gradle.internal.snapshot.MissingFileSnapshot
import org.gradle.internal.snapshot.TestSnapshotFixture
import org.gradle.internal.vfs.FileSystemAccess
import spock.lang.Issue

import java.nio.file.AccessDeniedException
import java.nio.file.Files
import java.nio.file.Paths
import java.time.Duration
Expand All @@ -47,11 +49,18 @@ import static org.gradle.internal.execution.ExecutionEngine.ExecutionOutcome.UP_
class AssignImmutableWorkspaceStepTest extends StepSpec<IdentityContext> implements TestSnapshotFixture {
def immutableWorkspace = file("immutable-workspace")
def temporaryWorkspace = file("temporary-workspace")
def duplicateTemporaryWorkspace = file("duplicate-temporary-workspace")
def workspace = Stub(ImmutableWorkspace) {
immutableLocation >> immutableWorkspace
withTemporaryWorkspace(_ as TemporaryWorkspaceAction) >> { TemporaryWorkspaceAction action ->
action.executeInTemporaryWorkspace(temporaryWorkspace)
}
withTemporaryWorkspace(_ as TemporaryWorkspaceAction)
>>
{ TemporaryWorkspaceAction action ->
action.executeInTemporaryWorkspace(temporaryWorkspace)
}
>>
{ TemporaryWorkspaceAction action ->
action.executeInTemporaryWorkspace(duplicateTemporaryWorkspace)
}
}

def deleter = Mock(Deleter)
Expand Down Expand Up @@ -163,6 +172,59 @@ class AssignImmutableWorkspaceStepTest extends StepSpec<IdentityContext> impleme
0 * _
}

@Issue("https://github.com/gradle/gradle/issues/27844")
def "falls back to duplicating temporary workspace when the original cannot be moved atomically"() {
def delegateExecution = Mock(ExecutionEngine.Execution)
def delegateDuration = Duration.ofSeconds(1)
def delegateOriginMetadata = Stub(OriginMetadata)
def delegateOutputFiles = ImmutableSortedMap.of()
def delegateOutputState = Stub(ExecutionOutputState) {
getOriginMetadata() >> delegateOriginMetadata
getOutputFilesProducedByWork() >> delegateOutputFiles
}
def delegateResult = Stub(CachingResult) {
getExecution() >> Try.successful(delegateExecution)
getDuration() >> delegateDuration
getAfterExecutionOutputState() >> Optional.of(delegateOutputState)
}

when:
step.execute(work, context)

then:
1 * fileSystemAccess.read(immutableWorkspace.absolutePath) >> Stub(MissingFileSnapshot) {
type >> FileType.Missing
}

then:
1 * fileSystemAccess.invalidate([temporaryWorkspace.absolutePath])

then:
1 * delegate.execute(work, _ as WorkspaceContext) >> { UnitOfWork work, WorkspaceContext delegateContext ->
assert delegateContext.workspace == temporaryWorkspace
temporaryWorkspace.file("output.txt").text = "output"
return delegateResult
}

then:
1 * immutableWorkspaceMetadataStore.storeWorkspaceMetadata(temporaryWorkspace, _)
1 * fileSystemAccess.moveAtomically(temporaryWorkspace.absolutePath, immutableWorkspace.absolutePath) >> { String from, String to ->
throw new AccessDeniedException("Simulate Windows keeping file locks open")
}

then:
1 * fileSystemAccess.moveAtomically(duplicateTemporaryWorkspace.absolutePath, immutableWorkspace.absolutePath) >>{ String from, String to ->
Files.move(Paths.get(from), Paths.get(to))
}

then:
1 * deleter.deleteRecursively(temporaryWorkspace)

then:
immutableWorkspace.file("output.txt").text == "output"
0 * _
}

def "keeps failed outputs in temporary workspace"() {
def delegateFailure = Mock(Exception)
def delegateDuration = Duration.ofSeconds(1)
Expand Down Expand Up @@ -213,9 +275,6 @@ class AssignImmutableWorkspaceStepTest extends StepSpec<IdentityContext> impleme
type >> FileType.Directory
}

def originalOutputs = ImmutableSortedMap.<String, FileSystemLocationSnapshot> of(
"output", originalOutputFileSnapshot
)
def changedOutputs = ImmutableSortedMap.<String, FileSystemLocationSnapshot> of(
"output", changedOutputFileSnapshot
)
Expand All @@ -235,7 +294,9 @@ class AssignImmutableWorkspaceStepTest extends StepSpec<IdentityContext> impleme

then:
def ex = thrown IllegalStateException
ex.message.startsWith("Workspace has been changed")
ex.message == "Immutable workspace contents have been modified: ${immutableWorkspace.absolutePath}. " +
"These workspace directories are not supposed to be modified once they are created. " +
"Deleting the directory in question can allow the content to be recreated."
0 * _
}
}

0 comments on commit f1b03c0

Please sign in to comment.