From d324a7116eea27d14d53e4f91e3b688c8ad0b07f Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Thu, 14 Sep 2023 16:47:32 +0100 Subject: [PATCH] Clear up a bunch of error handling code (#209) * Have `log.Fatal` shutdown the server before terminating the process. * Use structured logging rather than panicing on gRPC error. * Don't double-shut-down gRPC connection from the Java side. * Give a nice user-facing error if one directory declares multiple Java packages. --- java/gazelle/generate.go | 2 +- java/gazelle/lang.go | 18 ++++++++++++++++++ java/gazelle/private/javaparser/BUILD.bazel | 1 + java/gazelle/private/javaparser/javaparser.go | 6 ++++++ .../multiple_packages_in_one_dir/WORKSPACE | 0 .../expectedExitCode.txt | 1 + .../expectedStderr.txt | 2 ++ .../src/main/com/example/Class1.java | 3 +++ .../src/main/com/example/Class2.java | 3 +++ .../javaparser/generators/GrpcServer.java | 14 ++++++-------- 10 files changed, 41 insertions(+), 9 deletions(-) create mode 100644 java/gazelle/testdata/multiple_packages_in_one_dir/WORKSPACE create mode 100644 java/gazelle/testdata/multiple_packages_in_one_dir/expectedExitCode.txt create mode 100644 java/gazelle/testdata/multiple_packages_in_one_dir/expectedStderr.txt create mode 100644 java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class1.java create mode 100644 java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class2.java diff --git a/java/gazelle/generate.go b/java/gazelle/generate.go index 288d9fc5..02658f33 100644 --- a/java/gazelle/generate.go +++ b/java/gazelle/generate.go @@ -71,7 +71,7 @@ func (l javaLang) GenerateRules(args language.GenerateArgs) language.GenerateRes Files: javaFilenamesRelativeToPackage, }) if err != nil { - panic(err) + log.Fatal().Err(err).Str("package", args.Rel).Msg("Failed to parse package") } // We exclude intra-package imports to avoid self-dependencies. diff --git a/java/gazelle/lang.go b/java/gazelle/lang.go index d3cf014a..9dc2baef 100644 --- a/java/gazelle/lang.go +++ b/java/gazelle/lang.go @@ -65,6 +65,10 @@ func NewLanguage() language.Language { javaPackageCache: make(map[string]*java.Package), } + l.logger = l.logger.Hook(shutdownServerOnFatalLogHook{ + l: &l, + }) + l.Configurer = NewConfigurer(&l) l.Resolver = NewResolver(&l) @@ -163,3 +167,17 @@ func (l javaLang) AfterResolvingDeps(_ context.Context) { l.logger.Fatal().Msg("the java extension encontered errors that will create invalid build files") } } + +type shutdownServerOnFatalLogHook struct { + l *javaLang +} + +func (s shutdownServerOnFatalLogHook) Run(e *zerolog.Event, level zerolog.Level, message string) { + if s.l.parser == nil { + return + } + if level != zerolog.FatalLevel { + return + } + s.l.parser.ServerManager().Shutdown() +} diff --git a/java/gazelle/private/javaparser/BUILD.bazel b/java/gazelle/private/javaparser/BUILD.bazel index f93f4b2a..7cc99bc2 100644 --- a/java/gazelle/private/javaparser/BUILD.bazel +++ b/java/gazelle/private/javaparser/BUILD.bazel @@ -12,5 +12,6 @@ go_library( "//java/gazelle/private/sorted_set", "//java/gazelle/private/types", "@com_github_rs_zerolog//:zerolog", + "@org_golang_google_grpc//status", ], ) diff --git a/java/gazelle/private/javaparser/javaparser.go b/java/gazelle/private/javaparser/javaparser.go index ccc76894..5411f8ff 100644 --- a/java/gazelle/private/javaparser/javaparser.go +++ b/java/gazelle/private/javaparser/javaparser.go @@ -11,6 +11,7 @@ import ( "github.com/bazel-contrib/rules_jvm/java/gazelle/private/sorted_set" "github.com/bazel-contrib/rules_jvm/java/gazelle/private/types" "github.com/rs/zerolog" + "google.golang.org/grpc/status" ) type Runner struct { @@ -53,6 +54,11 @@ func (r Runner) ParsePackage(ctx context.Context, in *ParsePackageRequest) (*jav resp, err := r.rpc.ParsePackage(ctx, &pb.ParsePackageRequest{Rel: in.Rel, Files: in.Files}) if err != nil { + if grpcErr, ok := status.FromError(err); ok { + // gRPC is an implementation detail of the javaparser layer, and shouldn't be relied on by higher layers. + // Reformat gRPC-related details here, for more clear error messages. + return nil, fmt.Errorf("%s: %s", grpcErr.Code().String(), grpcErr.Message()) + } return nil, err } diff --git a/java/gazelle/testdata/multiple_packages_in_one_dir/WORKSPACE b/java/gazelle/testdata/multiple_packages_in_one_dir/WORKSPACE new file mode 100644 index 00000000..e69de29b diff --git a/java/gazelle/testdata/multiple_packages_in_one_dir/expectedExitCode.txt b/java/gazelle/testdata/multiple_packages_in_one_dir/expectedExitCode.txt new file mode 100644 index 00000000..56a6051c --- /dev/null +++ b/java/gazelle/testdata/multiple_packages_in_one_dir/expectedExitCode.txt @@ -0,0 +1 @@ +1 \ No newline at end of file diff --git a/java/gazelle/testdata/multiple_packages_in_one_dir/expectedStderr.txt b/java/gazelle/testdata/multiple_packages_in_one_dir/expectedStderr.txt new file mode 100644 index 00000000..79f239e9 --- /dev/null +++ b/java/gazelle/testdata/multiple_packages_in_one_dir/expectedStderr.txt @@ -0,0 +1,2 @@ +{"level":"warn","_c":"maven-resolver","error":"open %WORKSPACEPATH%/maven_install.json: no such file or directory","message":"not loading maven dependencies"} +{"level":"fatal","step":"GenerateRules","rel":"src/main/com/example","error":"InvalidArgument: Expected exactly one java package, but saw 2: com.example, example","package":"src/main/com/example","message":"Failed to parse package"} diff --git a/java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class1.java b/java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class1.java new file mode 100644 index 00000000..f3637a03 --- /dev/null +++ b/java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class1.java @@ -0,0 +1,3 @@ +package com.example; + +public class Class1 {} diff --git a/java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class2.java b/java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class2.java new file mode 100644 index 00000000..60a2550d --- /dev/null +++ b/java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class2.java @@ -0,0 +1,3 @@ +package example; + +public class Class2 {} diff --git a/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/GrpcServer.java b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/GrpcServer.java index 9975dffa..3b18b5a5 100644 --- a/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/GrpcServer.java +++ b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/GrpcServer.java @@ -7,6 +7,7 @@ import com.gazelle.java.javaparser.v0.Package.Builder; import com.gazelle.java.javaparser.v0.ParsePackageRequest; import com.gazelle.java.javaparser.v0.PerClassMetadata; +import com.google.common.base.Joiner; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Iterables; @@ -103,10 +104,7 @@ public void parsePackage( responseObserver.onNext(getImports(request)); responseObserver.onCompleted(); } catch (Exception ex) { - logger.error( - "Got Exception parsing package {}: {}", Paths.get(request.getRel()), ex.getMessage()); responseObserver.onError(ex); - responseObserver.onCompleted(); } finally { timeoutHandler.finishedRequest(); } @@ -132,11 +130,11 @@ private Package getImports(ParsePackageRequest request) { } Set packages = parser.getPackages(); if (packages.size() > 1) { - logger.error( - "Set of classes in {} should have only one package, instead is: {}", - request.getRel(), - packages); - throw new StatusRuntimeException(Status.INVALID_ARGUMENT); + throw new StatusRuntimeException( + Status.INVALID_ARGUMENT.withDescription( + String.format( + "Expected exactly one java package, but saw %d: %s", + packages.size(), Joiner.on(", ").join(packages)))); } else if (packages.isEmpty()) { logger.info( "Set of classes in {} has no package", Paths.get(request.getRel()).toAbsolutePath());