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

@@ -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.

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()
+}

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",
     ],
 )

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
 	}
 

java/gazelle/testdata/multiple_packages_in_one_dir/expectedExitCode.txt

@@ -0,0 +1 @@
+1

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"}

java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class1.java

@@ -0,0 +1,3 @@
+package com.example;
+
+public class Class1 {}

java/gazelle/testdata/multiple_packages_in_one_dir/src/main/com/example/Class2.java

@@ -0,0 +1,3 @@
+package example;
+
+public class Class2 {}

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<String> 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());