Skip to content

Commit

Permalink
ensure the gRPC LoggingInterceptor does not throw exceptions where th…
Browse files Browse the repository at this point in the history
…ey cannot be handled

This fixes a hang that I've struggled with for ages: when using `--build_event_json_file` to write a BEP file, enabling `--experimental_remote_grpc_log` causes the build to hang indefinitely after completion with a message of "Waiting for build events upload: JsonFormatFileTransport".

The default `--build_event_json_file_path_conversion` ends up doing an RPC call when closing the BEP file. Unfortunately, this call happens after the gRPC log file has already been closed. That exception in the LoggingInterceptor is uncatchable -- it [causes the RPC call to hang indefinitely](grpc/grpc-java#6107). Therefore, Bazel needs to handle and consume the exception so `onClose` always succeeds.

Closes #12416.

PiperOrigin-RevId: 340796218
  • Loading branch information
kevingessner authored and copybara-github committed Nov 5, 2020
1 parent cfb5b41 commit 74ca288
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/remote/util",
"//src/main/java/com/google/devtools/build/lib/util/io",
"//src/main/protobuf:remote_execution_log_java_proto",
"//third_party:flogger",
"//third_party:jsr305",
"//third_party/grpc:grpc-jar",
"//third_party/protobuf:protobuf_java",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import build.bazel.remote.execution.v2.ExecutionGrpc;
import build.bazel.remote.execution.v2.RequestMetadata;
import com.google.bytestream.ByteStreamGrpc;
import com.google.common.flogger.GoogleLogger;
import com.google.devtools.build.lib.clock.Clock;
import com.google.devtools.build.lib.remote.logging.RemoteExecutionLog.LogEntry;
import com.google.devtools.build.lib.remote.util.TracingMetadataUtils;
Expand All @@ -39,6 +40,8 @@

/** Client interceptor for logging details of certain gRPC calls. */
public class LoggingInterceptor implements ClientInterceptor {
private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();

private final AsynchronousFileOutputStream rpcLogFile;
private final Clock clock;

Expand Down Expand Up @@ -133,12 +136,22 @@ public void onMessage(RespT message) {
super.onMessage(message);
}

/**
* This method must not throw any exceptions! Doing so will cause the wrapped call to
* silently hang indefinitely: https://github.com/grpc/grpc-java/pull/6107
*/
@Override
public void onClose(Status status, Metadata trailers) {
entryBuilder.setEndTime(getCurrentTimestamp());
entryBuilder.setStatus(makeStatusProto(status));
entryBuilder.setDetails(handler.getDetails());
rpcLogFile.write(entryBuilder.build());
try {
rpcLogFile.write(entryBuilder.build());
} catch (RuntimeException e) {
// e.g. the log file is already closed.
logger.atWarning().withCause(e).log(
"Unable to write RPC log entry for %s", entryBuilder.build());
}
super.onClose(status, trailers);
}
},
Expand Down

0 comments on commit 74ca288

Please sign in to comment.