Skip to content

Commit

Permalink
Change the unchecked MissingDepException to a subclass of the check…
Browse files Browse the repository at this point in the history
…ed `ExecException`.

`MetadataProvider` must declare when it may request generating actions from skyframe. If it is true, we know that a null return from `getMetadata` on a `DerivedArtifact` should trigger a skyframe restart.

PiperOrigin-RevId: 354185668
  • Loading branch information
justinhorvitz authored and copybara-github committed Jan 27, 2021
1 parent ed15b45 commit e52b60d
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
* An exception indication that the execution of an action has failed OR could not be attempted OR
* could not be finished OR had something else wrong.
*
* <p>The five main kinds of failure are broadly defined as follows:
* <p>The six main kinds of failure are broadly defined as follows:
*
* <p>USER_INPUT which means it had something to do with what the user told us to do. This failure
* should satisfy the invariant that it would happen identically again if all other things are
Expand All @@ -42,6 +42,8 @@
* that went missing. Although this seems similar to ENVIRONMENT, Blaze may know how to fix this
* problem.
*
* <p>MISSING_DEP which means that a skyframe restart is necessary because a dependency was missing.
*
* <p>The class is a catch-all for both failures of actions and failures to evaluate actions
* properly.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.
package com.google.devtools.build.lib.actions;

import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.vfs.FileSystem;
import java.io.IOException;
Expand All @@ -34,6 +35,11 @@ public interface MetadataProvider {
* <p>The returned {@link FileArtifactValue} instance corresponds to the final target of a symlink
* and therefore must not have a type of {@link FileStateType#SYMLINK}.
*
* <p>If {@link #mayGetGeneratingActionsFromSkyframe} is {@code true} and the {@linkplain
* DerivedArtifact#getGeneratingActionKey generating action} is not immediately available, this
* method returns {@code null} to signify that a skyframe restart is necessary to obtain the
* metadata for the requested {@link Artifact.DerivedArtifact}.
*
* @param input the input to retrieve the digest for
* @return the artifact's digest or null if digest cannot be obtained (due to artifact
* non-existence, lookup errors, or any other reason)
Expand All @@ -59,4 +65,12 @@ public interface MetadataProvider {
default FileSystem getFileSystemForInputResolution() {
return null;
}

/**
* Indicates whether calls to {@link #getMetadata} with a {@link Artifact.DerivedArtifact} may
* require a skyframe lookup.
*/
default boolean mayGetGeneratingActionsFromSkyframe() {
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,23 @@
// limitations under the License.
package com.google.devtools.build.lib.actions;

import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;

/**
* Exception to be thrown if an action is missing Skyframe dependencies that it finds are missing
* during execution/input discovery.
* Exception to be thrown if an action failed to execute because it is missing Skyframe
* dependencies.
*
* <p>This is expected to be possible when {@link
* MetadataProvider#mayGetGeneratingActionsFromSkyframe} is {@code true}.
*/
public final class MissingDepException extends RuntimeException {}
public final class MissingDepExecException extends ExecException {

public MissingDepExecException() {
super("Missing skyframe dependency");
}

@Override
protected FailureDetail getFailureDetail(String message) {
throw new UnsupportedOperationException("MissingDepException should be handled");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import com.google.devtools.build.lib.actions.ArtifactFactory;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.MissingDepException;
import com.google.devtools.build.lib.actions.MissingDepExecException;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.concurrent.AbstractQueueVisitor;
Expand Down Expand Up @@ -599,11 +599,11 @@ public final void processAsync(
ActionExecutionContext actionExecutionContext,
Artifact grepIncludes)
throws IOException, ExecException, InterruptedException {
SkyFunction.Environment env = actionExecutionContext.getEnvironmentForDiscoveringInputs();
ImmutableSet<Artifact> pathHints;
if (parser.getHints() == null) {
pathHints = ImmutableSet.of();
} else {
SkyFunction.Environment env = actionExecutionContext.getEnvironmentForDiscoveringInputs();
pathHints = parser.getHints().getPathLevelHintedInclusions(quoteIncludePaths, env);
if (env.valuesMissing()) {
return;
Expand All @@ -617,7 +617,16 @@ public final void processAsync(
actionExecutionContext,
grepIncludes,
includeScanningHeaderData);
visitor.processInternal(mainSource, sources, cmdlineIncludes, includes, pathHints);

try {
visitor.processInternal(mainSource, sources, cmdlineIncludes, includes, pathHints);
} catch (MissingDepExecException e) {
// This happens when a skyframe restart is necessary. Callers are responsible for checking
// env.valuesMissing() as per this method's contract, so we can just ignore the exception.
if (!env.valuesMissing()) {
throw new IllegalStateException("Missing dep without skyframe request", e);
}
}
}

private static void checkForInterrupt(String operation, Object source)
Expand Down Expand Up @@ -721,7 +730,7 @@ void processInternal(
frontier = adjacent;
}
}
} catch (IOException | InterruptedException | ExecException | MissingDepException e) {
} catch (IOException | InterruptedException | ExecException e) {
// Careful: Do not leak visitation threads if we have an exception in the initial thread.
sync();
throw e;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.actions.LostInputsActionExecutionException;
import com.google.devtools.build.lib.actions.MissingDepException;
import com.google.devtools.build.lib.actions.MissingInputFileException;
import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.SpawnMetrics;
Expand Down Expand Up @@ -772,9 +771,6 @@ private ActionExecutionValue checkCacheAndExecuteIfNeeded(
state.discoveredInputs =
skyframeActionExecutor.discoverInputs(
action, actionLookupData, metadataHandler, env, state.actionFileSystem);
} catch (MissingDepException e) {
Preconditions.checkState(env.valuesMissing(), action);
return null;
}
discoveredInputsDuration = Duration.ofNanos(BlazeClock.nanoTime() - actionStartTime);
if (env.valuesMissing()) {
Expand Down

0 comments on commit e52b60d

Please sign in to comment.