diff --git a/WORKSPACE b/WORKSPACE index 471f5b3d..93901498 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -45,6 +45,7 @@ maven_install( name = "frozen_deps", artifacts = [ "com.google.code.findbugs:jsr305:3.0.2", + "com.google.errorprone:error_prone_annotations:2.11.0", "com.google.guava:guava:30.1.1-jre", "commons-cli:commons-cli:1.5.0", "io.grpc:grpc-api:1.40.0", diff --git a/frozen_deps_install.json b/frozen_deps_install.json index 0860ad16..3d843e42 100644 --- a/frozen_deps_install.json +++ b/frozen_deps_install.json @@ -1,7 +1,7 @@ { "dependency_tree": { "__AUTOGENERATED_FILE_DO_NOT_MODIFY_THIS_FILE_MANUALLY": "THERE_IS_NO_DATA_ONLY_ZUUL", - "__INPUT_ARTIFACTS_HASH": 1411780159, + "__INPUT_ARTIFACTS_HASH": -1395678455, "__RESOLVED_ARTIFACTS_HASH": 1576619931, "conflict_resolution": { "com.google.errorprone:error_prone_annotations:2.9.0": "com.google.errorprone:error_prone_annotations:2.11.0", diff --git a/java/gazelle/BUILD.bazel b/java/gazelle/BUILD.bazel index c4a4e8c7..10583f6c 100644 --- a/java/gazelle/BUILD.bazel +++ b/java/gazelle/BUILD.bazel @@ -12,6 +12,9 @@ go_library( "lang.go", "resolve.go", ], + data = [ + "//java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators:Main", + ], importpath = "github.com/bazel-contrib/rules_jvm/java/gazelle", visibility = ["//visibility:public"], deps = [ diff --git a/java/gazelle/configure.go b/java/gazelle/configure.go index 759860a8..bbdbd508 100644 --- a/java/gazelle/configure.go +++ b/java/gazelle/configure.go @@ -99,7 +99,11 @@ func (jc *Configurer) Configure(c *config.Config, rel string, f *rule.File) { } if jc.lang.parser == nil { - jc.lang.parser = javaparser.NewRunner(jc.lang.logger, c.RepoRoot, jc.lang.javaLogLevel) + runner, err := javaparser.NewRunner(jc.lang.logger, c.RepoRoot, jc.lang.javaLogLevel) + if err != nil { + jc.lang.logger.Fatal().Err(err).Msg("could not start javaparser") + } + jc.lang.parser = runner } if jc.lang.mavenResolver == nil { diff --git a/java/gazelle/lang.go b/java/gazelle/lang.go index dddbc252..9a52c611 100644 --- a/java/gazelle/lang.go +++ b/java/gazelle/lang.go @@ -116,3 +116,7 @@ func (l javaLang) Loads() []rule.LoadInfo { } func (l javaLang) Fix(c *config.Config, f *rule.File) {} + +func (l javaLang) DoneGeneratingRules() { + l.parser.ServerManager().Shutdown() +} diff --git a/java/gazelle/private/javaparser/BUILD.bazel b/java/gazelle/private/javaparser/BUILD.bazel index 6cbed567..1aa7587e 100644 --- a/java/gazelle/private/javaparser/BUILD.bazel +++ b/java/gazelle/private/javaparser/BUILD.bazel @@ -3,16 +3,13 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library") go_library( name = "javaparser", srcs = ["javaparser.go"], - data = ["//java/gazelle/private/javaparser/cmd/javaparser-wrapper"], importpath = "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser", visibility = ["//java/gazelle:__subpackages__"], deps = [ - "//java/gazelle/private/bazel", "//java/gazelle/private/java", - "//java/gazelle/private/javaparser/netutil", "//java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0:javaparser", + "//java/gazelle/private/servermanager", "//java/gazelle/private/sorted_set", "@com_github_rs_zerolog//:zerolog", - "@org_golang_google_grpc//:go_default_library", ], ) diff --git a/java/gazelle/private/javaparser/cmd/javaparser-wrapper/BUILD.bazel b/java/gazelle/private/javaparser/cmd/javaparser-wrapper/BUILD.bazel deleted file mode 100644 index 65803f89..00000000 --- a/java/gazelle/private/javaparser/cmd/javaparser-wrapper/BUILD.bazel +++ /dev/null @@ -1,23 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library") - -go_library( - name = "javaparser-wrapper_lib", - srcs = ["main.go"], - data = ["//java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators:Main"], - importpath = "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser/cmd/javaparser-wrapper", - visibility = ["//visibility:private"], - deps = [ - "//java/gazelle/private/bazel", - "//java/gazelle/private/javaparser/cmd/javaparser-wrapper/internal/activitytracker", - "//java/gazelle/private/javaparser/netutil", - "//java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0:javaparser", - "@com_github_rs_zerolog//:zerolog", - "@org_golang_google_grpc//:go_default_library", - ], -) - -go_binary( - name = "javaparser-wrapper", - embed = [":javaparser-wrapper_lib"], - visibility = ["//visibility:public"], -) diff --git a/java/gazelle/private/javaparser/cmd/javaparser-wrapper/internal/activitytracker/BUILD.bazel b/java/gazelle/private/javaparser/cmd/javaparser-wrapper/internal/activitytracker/BUILD.bazel deleted file mode 100644 index 6534eac9..00000000 --- a/java/gazelle/private/javaparser/cmd/javaparser-wrapper/internal/activitytracker/BUILD.bazel +++ /dev/null @@ -1,8 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") - -go_library( - name = "activitytracker", - srcs = ["tracker.go"], - importpath = "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser/cmd/javaparser-wrapper/internal/activitytracker", - visibility = ["//java/gazelle:__subpackages__"], -) diff --git a/java/gazelle/private/javaparser/cmd/javaparser-wrapper/internal/activitytracker/tracker.go b/java/gazelle/private/javaparser/cmd/javaparser-wrapper/internal/activitytracker/tracker.go deleted file mode 100644 index 882dc909..00000000 --- a/java/gazelle/private/javaparser/cmd/javaparser-wrapper/internal/activitytracker/tracker.go +++ /dev/null @@ -1,31 +0,0 @@ -package activitytracker - -import ( - "sync" - "time" -) - -type Tracker struct { - lastActivityLock sync.RWMutex - lastActivity time.Time -} - -func NewTracker() *Tracker { - return &Tracker{ - lastActivity: time.Now(), - } -} - -func (t *Tracker) Ping() { - t.lastActivityLock.Lock() - defer t.lastActivityLock.Unlock() - - t.lastActivity = time.Now() -} - -func (t *Tracker) IdleSince(q time.Time) bool { - t.lastActivityLock.RLock() - defer t.lastActivityLock.RUnlock() - - return t.lastActivity.Before(q) -} diff --git a/java/gazelle/private/javaparser/cmd/javaparser-wrapper/main.go b/java/gazelle/private/javaparser/cmd/javaparser-wrapper/main.go deleted file mode 100644 index 5fa7b7f7..00000000 --- a/java/gazelle/private/javaparser/cmd/javaparser-wrapper/main.go +++ /dev/null @@ -1,232 +0,0 @@ -package main - -import ( - "context" - "flag" - "fmt" - "net" - "os" - "os/exec" - "os/signal" - "strings" - "time" - - "github.com/bazel-contrib/rules_jvm/java/gazelle/private/bazel" - "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser/cmd/javaparser-wrapper/internal/activitytracker" - "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser/netutil" - pb "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0" - "github.com/rs/zerolog" - "google.golang.org/grpc" -) - -// runnerTimout is the time after which the runner will die. -const runnerTimout = 30 * time.Second - -type arrayFlags []string - -func (i *arrayFlags) String() string { - return strings.Join(*i, ",") -} - -func (i *arrayFlags) Set(value string) error { - *i = append(*i, value) - return nil -} - -func main() { - var addr, workspacePath string - var jvmArgs arrayFlags - - flag.StringVar(&addr, "addr", "", "Address to listen") - flag.StringVar(&workspacePath, "workspace", "", "Where is the workspace") - flag.Var(&jvmArgs, "jvm_arg", "Pass to the java command itself. may contain spaces. Can be used multiple times.") - flag.Parse() - - javaLevel := "info" - for _, arg := range jvmArgs { - if strings.HasPrefix(arg, "-Dorg.slf4j.simpleLogger.defaultLogLevel=") { - javaLevel = strings.TrimPrefix(arg, "-Dorg.slf4j.simpleLogger.defaultLogLevel=") - } - } - - goLevel, err := zerolog.ParseLevel(javaLevel) - if err != nil { - panic(err.Error()) - } - - logger := zerolog.New(zerolog.ConsoleWriter{Out: os.Stderr}). - With(). - Timestamp(). - Caller(). - Logger(). - Level(goLevel) - - c := make(chan os.Signal, 1) - signal.Notify(c, os.Interrupt) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - if workspacePath == "" { - logger.Error().Msg("missing workspace parameter") - flag.Usage() - os.Exit(1) - } - - binPath, err := findBinary(logger) - if err != nil { - logger.Fatal().Err(err).Msg("could not find BFG") - } - - realBFGAddr, err := netutil.RandomAddr() - if err != nil { - logger.Fatal().Msg("could not get an adresse for BFG") - } - - _, port, err := net.SplitHostPort(realBFGAddr) - if err != nil { - logger.Fatal().Err(err).Msg("could not get port from address") - } - - var args []string - for _, arg := range jvmArgs { - args = append(args, "--jvm_flag="+arg) - } - args = append( - args, - "--server-port", port, - "--workspace", workspacePath, - ) - - cmd := exec.Command(binPath, args...) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - logger.Debug().Strs("args", cmd.Args).Msg("bfg-server args") - if err := cmd.Start(); err != nil { - logger.Fatal().Err(err).Msg("could not start command") - } - defer func() { - logger.Debug().Msg("stopping real BFG") - if err := cmd.Process.Kill(); err != nil { - logger.Error().Err(err).Msg("failed to stop real BFG") - } - }() - - conn, err := grpc.Dial(realBFGAddr, grpc.WithInsecure(), grpc.WithBlock()) - if err != nil { - logger.Fatal().Err(err).Msg("did not connect") - } - defer conn.Close() - - srv := newServer(logger, workspacePath, pb.NewJavaParserClient(conn)) - at := activitytracker.NewTracker() - s := grpc.NewServer(grpc.UnaryInterceptor(activityTrackerUSI(at))) - pb.RegisterJavaParserServer(s, srv) - - ln, err := net.Listen("tcp", addr) - if err != nil { - logger.Fatal().Err(err).Msg("failed to listen") - } - defer ln.Close() - - logger.Debug().Stringer("addr", ln.Addr()).Msg("javaparser-wrapper server listening") - - go func() { - if err := s.Serve(ln); err != nil { - logger.Fatal().Err(err).Msg("failed to serve") - } - }() - - go func() { - t := time.NewTicker(5 * time.Second) - defer t.Stop() - - for { - select { - case <-ctx.Done(): - return - - case ts := <-t.C: - logger.Debug().Str("ts", ts.String()).Msg("reaper ticked") - past := ts.Add(-runnerTimout) - if at.IdleSince(past) { - logger.Debug().Str("ts", ts.String()).Str("past", past.String()).Msg("reaper stopping process") - cancel() - } - } - } - }() - - go func() { - oscall := <-c - logger.Debug().Str("syscall", fmt.Sprintf("%+v", oscall)).Msg("syscall") - cancel() - }() - - <-ctx.Done() - logger.Debug().Msg("graceful stop started") - s.GracefulStop() - logger.Debug().Msg("graceful stop done") -} - -func activityTrackerUSI(at *activitytracker.Tracker) grpc.UnaryServerInterceptor { - return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) { - at.Ping() - return handler(ctx, req) - } -} - -type server struct { - logger zerolog.Logger - real pb.JavaParserClient - workspace string -} - -func newServer(logger zerolog.Logger, workspace string, real pb.JavaParserClient) *server { - return &server{ - logger: logger.With().Str("_c", "server").Logger(), - real: real, - workspace: workspace, - } -} - -func (s *server) ParsePackage(ctx context.Context, in *pb.ParsePackageRequest) (*pb.Package, error) { - s.logger.Debug(). - Str("name", "ParsePackage"). - Msg("RPC") - - pkg, err := s.real.ParsePackage(ctx, in) - if err != nil { - return nil, err - } - - return pkg, nil -} - -// findBinary finds the build_file_generator. -// -// forked and simplified from bazel.FindBinary. -func findBinary(logger zerolog.Logger) (string, error) { - entries, err := bazel.ListRunfiles() - if err != nil { - return "", err - } - - wValues := make(map[string]bool) - - for _, entry := range entries { - if _, ok := wValues[entry.Workspace]; !ok { - logger.Debug().Msgf("new workspace: %#v", entry.Workspace) - wValues[entry.Workspace] = true - } - if entry.Workspace != "contrib_rules_jvm" { - continue - } - if entry.ShortPath != "java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/Main" { - continue - } - logger.Debug().Msgf("entry: %#v", entry) - return entry.Path, nil - } - - return "", fmt.Errorf("could not find javaparser") -} diff --git a/java/gazelle/private/javaparser/javaparser.go b/java/gazelle/private/javaparser/javaparser.go index 6986737f..a25d8142 100644 --- a/java/gazelle/private/javaparser/javaparser.go +++ b/java/gazelle/private/javaparser/javaparser.go @@ -2,71 +2,39 @@ package javaparser import ( "context" - "os" - "os/exec" + "fmt" "time" - "github.com/bazel-contrib/rules_jvm/java/gazelle/private/bazel" "github.com/bazel-contrib/rules_jvm/java/gazelle/private/java" - "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser/netutil" pb "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0" + "github.com/bazel-contrib/rules_jvm/java/gazelle/private/servermanager" "github.com/bazel-contrib/rules_jvm/java/gazelle/private/sorted_set" "github.com/rs/zerolog" - "google.golang.org/grpc" ) type Runner struct { - logger zerolog.Logger - repoRoot string - - rpc pb.JavaParserClient - rpcProcess *os.Process - conn *grpc.ClientConn + logger zerolog.Logger + rpc pb.JavaParserClient + serverManager *servermanager.ServerManager } -func NewRunner(logger zerolog.Logger, repoRoot string, javaLogLevel string) *Runner { - wrapperPath, found := bazel.FindBinary("java/gazelle/private/javaparser/cmd/javaparser-wrapper", "javaparser-wrapper") - if !found { - logger.Fatal().Msg("could not find javaparser-wrapper") - } - - r := Runner{ - logger: logger.With().Str("_c", "javaparser-runnerwrapper").Logger(), - repoRoot: repoRoot, - } +func NewRunner(logger zerolog.Logger, repoRoot string, javaLogLevel string) (*Runner, error) { + serverManager := servermanager.New(repoRoot, javaLogLevel) - addr, err := netutil.RandomAddr() + conn, err := serverManager.Connect() if err != nil { - logger.Fatal().Msg("could not get an address for javaparser-wrapper") - } - - cmd := exec.Command( - wrapperPath, - "--jvm_arg=-Dorg.slf4j.simpleLogger.defaultLogLevel="+javaLogLevel, - "--addr="+addr, - "--workspace="+repoRoot, - ) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - r.logger.Debug().Strs("args", cmd.Args).Msg("javaparser args") - if err := cmd.Start(); err != nil { - r.logger.Fatal().Err(err).Msg("could not start command") + return nil, fmt.Errorf("failed to start / connect to javaparser server: %w", err) } - // FIXME we do not have a destructor of the language to stop this - r.logger.Debug(). - Int("pid", cmd.Process.Pid). - Msg("gazelle does not know how to kill javaparser, do it yourself") - r.rpcProcess = cmd.Process - - conn, err := grpc.Dial(addr, grpc.WithInsecure(), grpc.WithBlock(), grpc.WithTimeout(30*time.Second)) - if err != nil { - r.logger.Fatal().Err(err).Msg("error connecting to javaparser-wrapper") - } - r.conn = conn - r.rpc = pb.NewJavaParserClient(r.conn) + return &Runner{ + logger: logger.With().Str("_c", "javaparser").Logger(), + rpc: pb.NewJavaParserClient(conn), + serverManager: serverManager, + }, nil +} - return &r +func (r *Runner) ServerManager() *servermanager.ServerManager { + return r.serverManager } type ParsePackageRequest struct { diff --git a/java/gazelle/private/javaparser/netutil/BUILD.bazel b/java/gazelle/private/javaparser/netutil/BUILD.bazel deleted file mode 100644 index d272e67f..00000000 --- a/java/gazelle/private/javaparser/netutil/BUILD.bazel +++ /dev/null @@ -1,8 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") - -go_library( - name = "netutil", - srcs = ["netutil.go"], - importpath = "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser/netutil", - visibility = ["//java/gazelle:__subpackages__"], -) diff --git a/java/gazelle/private/javaparser/netutil/netutil.go b/java/gazelle/private/javaparser/netutil/netutil.go deleted file mode 100644 index e179bbb4..00000000 --- a/java/gazelle/private/javaparser/netutil/netutil.go +++ /dev/null @@ -1,16 +0,0 @@ -package netutil - -import ( - "fmt" - "net" -) - -func RandomAddr() (string, error) { - const addr = "127.0.0.1:0" - l, err := net.Listen("tcp", addr) - if err != nil { - return "", fmt.Errorf("could not listen to %s: %w", addr, err) - } - defer l.Close() - return l.Addr().String(), nil -} diff --git a/java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0/javaparser.proto b/java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0/javaparser.proto index 036f544d..f76f70af 100644 --- a/java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0/javaparser.proto +++ b/java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0/javaparser.proto @@ -48,3 +48,11 @@ message Package { message PerClassMetadata { repeated string annotation_class_names = 1; } + +service Lifecycle { + rpc Shutdown(ShutdownRequest) returns (ShutdownResponse) {} +} + +message ShutdownRequest {} + +message ShutdownResponse {} diff --git a/java/gazelle/private/patches/BUILD.bazel b/java/gazelle/private/patches/BUILD.bazel deleted file mode 100644 index e69de29b..00000000 diff --git a/java/gazelle/private/patches/bazel_gazelle-1324-allow-configuring-timeout-of-generation-tests.patch b/java/gazelle/private/patches/bazel_gazelle-1324-allow-configuring-timeout-of-generation-tests.patch deleted file mode 100644 index bbb75148..00000000 --- a/java/gazelle/private/patches/bazel_gazelle-1324-allow-configuring-timeout-of-generation-tests.patch +++ /dev/null @@ -1,107 +0,0 @@ -commit d3f585c6234bfba82e6d6ec23a7f9e280a2179eb -Author: Daniel Wagner-Hall -Date: Mon Aug 29 10:52:57 2022 -0400 - - Allow configuring timeout of generation tests - -diff --git a/extend.md b/extend.md -index 4110cb0..df6cc35 100644 ---- a/extend.md -+++ b/extend.md -@@ -144,7 +144,8 @@ proto extension stores metadata in hidden attributes of generated - ## gazelle_generation_test - -
--gazelle_generation_test(name, gazelle_binary, test_data, build_in_suffix, build_out_suffix)
-+gazelle_generation_test(name, gazelle_binary, test_data, build_in_suffix, build_out_suffix,
-+                        gazelle_timeout_seconds)
- 
- - gazelle_generation_test is a macro for testing gazelle against workspaces. -@@ -178,5 +179,6 @@ To update the expected files, run `UPDATE_SNAPSHOTS=true bazel run //path/to:the - | test_data | A list of target of the test data files you will pass to the test. This can be a https://bazel.build/reference/be/general#filegroup. | none | - | build_in_suffix | The suffix for the input BUILD.bazel files. Defaults to .in. By default, will use files named BUILD.in as the BUILD files before running gazelle. | ".in" | - | build_out_suffix | The suffix for the expected BUILD.bazel files after running gazelle. Defaults to .out. By default, will use files named check the results of the gazelle run against files named BUILD.out. | ".out" | -+| gazelle_timeout_seconds |

-

| 2 | - - -diff --git a/internal/generationtest/generation_test.go b/internal/generationtest/generation_test.go -index a0b4d41..ebdb77d 100644 ---- a/internal/generationtest/generation_test.go -+++ b/internal/generationtest/generation_test.go -@@ -4,6 +4,7 @@ import ( - "flag" - "path/filepath" - "testing" -+ "time" - - "github.com/bazelbuild/bazel-gazelle/testtools" - "github.com/bazelbuild/rules_go/go/tools/bazel" -@@ -15,6 +16,7 @@ var ( - " By default, will use files named BUILD.in as the BUILD files before running gazelle.") - buildOutSuffix = flag.String("build_out_suffix", ".out", "The suffix on the expected BUILD.bazel files after running gazelle. Defaults to .out. "+ - " By default, will use files named BUILD.out as the expected results of the gazelle run.") -+ timeoutSeconds = flag.Int("timeout_seconds", 2, "Number of seconds to allow the gazelle process to run before killing.") - ) - - // TestFullGeneration runs the gazelle binary on a few example -@@ -51,6 +53,7 @@ func TestFullGeneration(t *testing.T) { - GazelleBinaryPath: absoluteGazelleBinary, - BuildInSuffix: *buildInSuffix, - BuildOutSuffix: *buildOutSuffix, -+ Timeout: time.Duration(*timeoutSeconds) * time.Second, - }) - } - } -diff --git a/internal/generationtest/generationtest.bzl b/internal/generationtest/generationtest.bzl -index 396a59a..97ae1ac 100644 ---- a/internal/generationtest/generationtest.bzl -+++ b/internal/generationtest/generationtest.bzl -@@ -4,7 +4,7 @@ Test for generating rules from gazelle. - - load("@io_bazel_rules_go//go:def.bzl", "go_test") - --def gazelle_generation_test(name, gazelle_binary, test_data, build_in_suffix = ".in", build_out_suffix = ".out"): -+def gazelle_generation_test(name, gazelle_binary, test_data, build_in_suffix = ".in", build_out_suffix = ".out", gazelle_timeout_seconds = 2): - """ - gazelle_generation_test is a macro for testing gazelle against workspaces. - -@@ -35,6 +35,7 @@ def gazelle_generation_test(name, gazelle_binary, test_data, build_in_suffix = " - By default, will use files named BUILD.in as the BUILD files before running gazelle. - build_out_suffix: The suffix for the expected BUILD.bazel files after running gazelle. Defaults to .out. - By default, will use files named check the results of the gazelle run against files named BUILD.out. -+ timeout_seconds: Number of seconds to allow the gazelle process to run before killing. - """ - go_test( - name = name, -@@ -47,6 +48,7 @@ def gazelle_generation_test(name, gazelle_binary, test_data, build_in_suffix = " - "-gazelle_binary_path=$(rootpath %s)" % gazelle_binary, - "-build_in_suffix=%s" % build_in_suffix, - "-build_out_suffix=%s" % build_out_suffix, -+ "-timeout_seconds=%d" % gazelle_timeout_seconds, - ], - data = test_data + [ - gazelle_binary, -diff --git a/testtools/files.go b/testtools/files.go -index df650f8..8da93ac 100644 ---- a/testtools/files.go -+++ b/testtools/files.go -@@ -160,6 +160,9 @@ type TestGazelleGenerationArgs struct { - // BuildOutSuffix is the suffix for all test output build files. Includes the ".". - // Default: ".out", so out BUILD files should be named BUILD.out. - BuildOutSuffix string -+ -+ // Timeout is the duration after which the generation process will be killed. -+ Timeout time.Duration - } - - var ( -@@ -322,7 +325,7 @@ Run %s to update BUILD.out and expected{Stdout,Stderr,ExitCode}.txt files. - } - }() - -- ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) -+ ctx, cancel := context.WithTimeout(context.Background(), args.Timeout) - defer cancel() - cmd := exec.CommandContext(ctx, args.GazelleBinaryPath, config.Args...) - cmd.Stdout = &stdout diff --git a/java/gazelle/private/servermanager/BUILD.bazel b/java/gazelle/private/servermanager/BUILD.bazel new file mode 100644 index 00000000..e7255a56 --- /dev/null +++ b/java/gazelle/private/servermanager/BUILD.bazel @@ -0,0 +1,14 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "servermanager", + srcs = ["servermanager.go"], + data = ["//java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators:Main"], + importpath = "github.com/bazel-contrib/rules_jvm/java/gazelle/private/servermanager", + visibility = ["//java/gazelle:__subpackages__"], + deps = [ + "//java/gazelle/private/bazel", + "//java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0:javaparser", + "@org_golang_google_grpc//:go_default_library", + ], +) diff --git a/java/gazelle/private/servermanager/servermanager.go b/java/gazelle/private/servermanager/servermanager.go new file mode 100644 index 00000000..f8924d18 --- /dev/null +++ b/java/gazelle/private/servermanager/servermanager.go @@ -0,0 +1,130 @@ +package servermanager + +import ( + "context" + "errors" + "fmt" + "io/ioutil" + "os" + "os/exec" + "path/filepath" + "strconv" + "sync" + "time" + + "github.com/bazel-contrib/rules_jvm/java/gazelle/private/bazel" + pb "github.com/bazel-contrib/rules_jvm/java/gazelle/private/javaparser/proto/gazelle/java/javaparser/v0" + "google.golang.org/grpc" +) + +type ServerManager struct { + workspace string + javaLogLevel string + + mu sync.Mutex + conn *grpc.ClientConn +} + +func New(workspace, javaLogLevel string) *ServerManager { + return &ServerManager{ + workspace: workspace, + javaLogLevel: javaLogLevel, + } +} + +func (m *ServerManager) Connect() (*grpc.ClientConn, error) { + m.mu.Lock() + defer m.mu.Unlock() + + if m.conn != nil { + return m.conn, nil + } + + logLevelFlag := fmt.Sprintf("-Dorg.slf4j.simpleLogger.defaultLogLevel=%s", m.javaLogLevel) + + javaParserPath, found := bazel.FindBinary("java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators", "Main") + if !found { + return nil, fmt.Errorf("failed to find javaparser in runfiles") + } + + dir, err := ioutil.TempDir(os.Getenv("TMPDIR"), "gazelle-javaparser") + if err != nil { + return nil, fmt.Errorf("failed to create tmpdir to start javaparser server: %w", err) + } + portFilePath := filepath.Join(dir, "port") + + cmd := exec.Command( + javaParserPath, + "--jvm_flag="+logLevelFlag, + "--server-port-file-path", portFilePath, + "--workspace", m.workspace, + "--idle-timeout", "30") + // Send JavaParser stdout to stderr for two reasons: + // 1. We don't want to pollute our own stdout + // 2. Java does some output buffer sniffing where it will block its own progress until the + // stdout buffer is read from, whereas stderr is unbuffered so doesn't hit this issue. + cmd.Stdout = os.Stderr + cmd.Stderr = os.Stderr + if err := cmd.Start(); err != nil { + return nil, fmt.Errorf("failed to start javaparser sever: %w", err) + } + + port, err := readPort(portFilePath) + if err != nil { + return nil, fmt.Errorf("failed to read port from javaparser server - maybe it crashed: %w", err) + } + + addr := fmt.Sprintf("localhost:%d", port) + + conn, err := grpc.DialContext(context.Background(), addr, grpc.WithInsecure(), grpc.WithBlock()) + if err != nil { + return nil, fmt.Errorf("failed to connect to javaparser server: %w", err) + } + + m.conn = conn + + return conn, nil +} + +func readPort(path string) (int32, error) { + startTime := time.Now() + for { + if time.Now().Sub(startTime) > 10*time.Second { + return 0, fmt.Errorf("timed out waiting for port file to be written by javaparser server") + } + + bs, err := ioutil.ReadFile(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + time.Sleep(10 * time.Millisecond) + continue + } + return 0, err + } else { + portStr := string(bs) + port, err := strconv.ParseInt(portStr, 10, 32) + if err != nil { + return 0, fmt.Errorf("error parsing port (%q) written by javaparser server: %w", portStr, err) + } + return int32(port), nil + } + } +} + +func (m *ServerManager) Shutdown() { + m.mu.Lock() + defer m.mu.Unlock() + + if m.conn == nil { + return + } + + cc := pb.NewLifecycleClient(m.conn) + + // If shutdown returns an error, there's really nothing for us to do. + cc.Shutdown(context.Background(), &pb.ShutdownRequest{}) + + m.conn.Close() + + m.conn = nil +} diff --git a/java/private/contrib_rules_jvm_deps.zip b/java/private/contrib_rules_jvm_deps.zip index cc9de6e3..68fcb879 100644 Binary files a/java/private/contrib_rules_jvm_deps.zip and b/java/private/contrib_rules_jvm_deps.zip differ diff --git a/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/BUILD.bazel b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/BUILD.bazel index 46160167..3b9a15e5 100644 --- a/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/BUILD.bazel +++ b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/BUILD.bazel @@ -6,7 +6,9 @@ java_library( "ClasspathParser.java", "GrpcServer.java", "JavaIdentifier.java", + "LifecycleService.java", "Main.java", + "TimeoutHandler.java", ], visibility = ["//java/test/com/github/bazel_contrib/contrib_rules_jvm/javaparser:__subpackages__"], runtime_deps = [ @@ -22,6 +24,7 @@ java_library( "@contrib_rules_jvm_deps//:io_grpc_grpc_services", "@contrib_rules_jvm_deps//:io_grpc_grpc_stub", "@contrib_rules_jvm_deps//:org_slf4j_slf4j_api", + "@maven//:com_google_errorprone_error_prone_annotations", ], ) 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 bfa2c4b6..4f569c9c 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 @@ -1,5 +1,7 @@ package com.github.bazel_contrib.contrib_rules_jvm.javaparser.generators; +import static java.nio.file.StandardCopyOption.ATOMIC_MOVE; + import com.gazelle.java.javaparser.v0.JavaParserGrpc; import com.gazelle.java.javaparser.v0.Package; import com.gazelle.java.javaparser.v0.Package.Builder; @@ -13,6 +15,8 @@ import io.grpc.protobuf.services.ProtoReflectionService; import io.grpc.stub.StreamObserver; import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; @@ -27,16 +31,19 @@ public class GrpcServer { private static final Logger logger = LoggerFactory.getLogger(GrpcServer.class); - private final int port; + private final Path serverPortFilePath; + private final TimeoutHandler timeoutHandler; private final Server server; /** Create a BuildFileGenerator server using serverBuilder as a base and features as data. */ - public GrpcServer(int port, Path workspace) { - this.port = port; - ServerBuilder serverBuilder = ServerBuilder.forPort(port); + public GrpcServer(Path serverPortFilePath, Path workspace, TimeoutHandler timeoutHandler) { + this.serverPortFilePath = serverPortFilePath; + this.timeoutHandler = timeoutHandler; + ServerBuilder serverBuilder = ServerBuilder.forPort(0); this.server = serverBuilder - .addService(new GrpcService(workspace)) + .addService(new GrpcService(workspace, timeoutHandler)) + .addService(new LifecycleService()) .addService(ProtoReflectionService.newInstance()) .build(); } @@ -44,28 +51,31 @@ public GrpcServer(int port, Path workspace) { /** Start serving requests. */ public void start() throws IOException { server.start(); - logger.debug("Server started, listening on {}", port); + + // Atomically write our server port to a file so that a reading process can't do a partial read. + Path tmpPath = serverPortFilePath.resolveSibling(serverPortFilePath.getFileName() + ".tmp"); + Files.write(tmpPath, String.format("%d", server.getPort()).getBytes(StandardCharsets.UTF_8)); + Files.move(tmpPath, serverPortFilePath, ATOMIC_MOVE); + + logger.debug("Server started, listening on {}", server.getPort()); Runtime.getRuntime() .addShutdownHook( new Thread() { @Override public void run() { - // Use stderr here since the logger may have been reset by its JVM - // shutdown hook. - System.err.println("*** shutting down gRPC server since JVM is shutting down"); + timeoutHandler.cancelOutstanding(); try { GrpcServer.this.stop(); } catch (InterruptedException e) { e.printStackTrace(System.err); } - System.err.println("*** server shut down"); } }); } /** Stop serving requests and shutdown resources. */ public void stop() throws InterruptedException { - server.shutdown().awaitTermination(30, TimeUnit.SECONDS); + server.shutdownNow().awaitTermination(30, TimeUnit.SECONDS); } /** Await termination on the main thread since the grpc library uses daemon threads. */ @@ -76,24 +86,29 @@ public void blockUntilShutdown() throws InterruptedException { private static class GrpcService extends JavaParserGrpc.JavaParserImplBase { private final Path workspace; + private final TimeoutHandler timeoutHandler; - GrpcService(Path workspace) { + GrpcService(Path workspace, TimeoutHandler timeoutHandler) { this.workspace = workspace; + this.timeoutHandler = timeoutHandler; } @Override public void parsePackage( ParsePackageRequest request, StreamObserver responseObserver) { - logger.debug("Got request, now processing"); + timeoutHandler.startedRequest(); + try { 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(); } - responseObserver.onCompleted(); - logger.debug("Finished processing request"); } private Package getImports(ParsePackageRequest request) { diff --git a/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/LifecycleService.java b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/LifecycleService.java new file mode 100644 index 00000000..752aa986 --- /dev/null +++ b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/LifecycleService.java @@ -0,0 +1,14 @@ +package com.github.bazel_contrib.contrib_rules_jvm.javaparser.generators; + +import com.gazelle.java.javaparser.v0.LifecycleGrpc; +import com.gazelle.java.javaparser.v0.ShutdownRequest; +import com.gazelle.java.javaparser.v0.ShutdownResponse; +import io.grpc.stub.StreamObserver; + +public class LifecycleService extends LifecycleGrpc.LifecycleImplBase { + + @Override + public void shutdown(ShutdownRequest request, StreamObserver responseObserver) { + System.exit(0); + } +} diff --git a/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/Main.java b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/Main.java index f7adf992..06e2c434 100644 --- a/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/Main.java +++ b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/Main.java @@ -3,6 +3,7 @@ import java.io.IOException; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.concurrent.ScheduledThreadPoolExecutor; import org.apache.commons.cli.CommandLine; import org.apache.commons.cli.CommandLineParser; import org.apache.commons.cli.DefaultParser; @@ -20,11 +21,14 @@ public class Main { public static void main(String[] args) throws IOException, InterruptedException { line = commandLineOptions(args); Main main = new Main(); - main.runServer(); + + TimeoutHandler timeoutHander = + new TimeoutHandler(new ScheduledThreadPoolExecutor(1), main.idleTimeout()); + main.runServer(timeoutHander); } - public void runServer() throws InterruptedException, IOException { - GrpcServer gRPCServer = new GrpcServer(serverPort(), workspace()); + public void runServer(TimeoutHandler timeoutHandler) throws InterruptedException, IOException { + GrpcServer gRPCServer = new GrpcServer(serverPortFilePath(), workspace(), timeoutHandler); gRPCServer.start(); gRPCServer.blockUntilShutdown(); } @@ -35,10 +39,15 @@ private Path workspace() { : Paths.get(""); } - private int serverPort() { - return line.hasOption("server-port") - ? Integer.decode(line.getOptionValue("server-port")) - : 8980; + private Path serverPortFilePath() { + return Paths.get(line.getOptionValue("server-port-file-path")); + } + + // <=0 means don't timeout. + private int idleTimeout() { + return line.hasOption("idle-timeout") + ? Integer.decode(line.getOptionValue("idle-timeout")) + : -1; } private static CommandLine commandLineOptions(String[] args) { @@ -47,8 +56,13 @@ private static CommandLine commandLineOptions(String[] args) { options.addOption(new Option("h", "help", false, "This help message")); options.addOption(new Option(null, "workspace", true, "Workspace root")); + options.addOption(new Option(null, "server-port-file-path", true, "TODO")); options.addOption( - new Option(null, "server-port", true, "Port to connect to the gRPC server (default 8980)")); + new Option( + null, + "idle-timeout", + true, + "Number of seconds after no gRPC activity to terminate self")); try { line = parser.parse(options, args); diff --git a/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/TimeoutHandler.java b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/TimeoutHandler.java new file mode 100644 index 00000000..638648c1 --- /dev/null +++ b/java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/TimeoutHandler.java @@ -0,0 +1,71 @@ +package com.github.bazel_contrib.contrib_rules_jvm.javaparser.generators; + +import com.google.errorprone.annotations.concurrent.GuardedBy; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class TimeoutHandler { + private static final Logger logger = LoggerFactory.getLogger(TimeoutHandler.class); + + private final ScheduledExecutorService executor; + private final int timeoutSeconds; + private final AtomicInteger inFlightRequests = new AtomicInteger(0); + + private final Object lastFutureLock = new Object(); + + @GuardedBy("lastFutureLock") + private ScheduledFuture lastFuture = null; + + public TimeoutHandler(ScheduledExecutorService executor, int timeoutSeconds) { + this.executor = executor; + this.timeoutSeconds = timeoutSeconds; + schedule(); + } + + public void startedRequest() { + this.inFlightRequests.getAndIncrement(); + } + + public void finishedRequest() { + this.inFlightRequests.getAndDecrement(); + schedule(); + } + + void schedule() { + if (this.timeoutSeconds <= 0) { + return; + } + + synchronized (lastFutureLock) { + ScheduledFuture last = this.lastFuture; + if (last != null) { + last.cancel(true); + } + this.lastFuture = + this.executor.schedule( + () -> { + if (inFlightRequests.get() == 0) { + logger.debug( + "Saw no requests in flight after {} seconds, terminating.", timeoutSeconds); + System.exit(0); + } + }, + timeoutSeconds, + TimeUnit.SECONDS); + } + } + + public void cancelOutstanding() { + synchronized (lastFutureLock) { + if (lastFuture != null) { + lastFuture.cancel(true); + lastFuture = null; + } + } + this.executor.shutdownNow(); + } +} diff --git a/java/test/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/BUILD.bazel b/java/test/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/BUILD.bazel index 7c59dab4..1c7ac3cb 100644 --- a/java/test/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/BUILD.bazel +++ b/java/test/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators/BUILD.bazel @@ -25,7 +25,6 @@ java_test_suite( deps = [ "//java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/file", "//java/src/com/github/bazel_contrib/contrib_rules_jvm/javaparser/generators", - "@contrib_rules_jvm_deps//:com_github_javaparser_javaparser_core", "@contrib_rules_jvm_deps//:junit_junit", "@contrib_rules_jvm_deps//:org_slf4j_slf4j_api", "@frozen_deps//:com_github_spotbugs_spotbugs_annotations", # keep diff --git a/repositories.bzl b/repositories.bzl index c82a5837..5d1fa8fa 100644 --- a/repositories.bzl +++ b/repositories.bzl @@ -59,15 +59,10 @@ def contrib_rules_jvm_gazelle_deps(): maybe( http_archive, name = "bazel_gazelle", - sha256 = "b794b5c4af78574e4bf574c6b2336bbffbdfeadfaae1c14c6184f154e44fdb47", - strip_prefix = "bazel-gazelle-622d8889c63227a71d6b393ba5e3a8b8a6761466", + sha256 = "dc4ed4f6cbb95abed45e3e3b3b8cb28de16696617a8ab90141c73920a186bb3d", + strip_prefix = "bazel-gazelle-05f5493ca9b74f037dde928721dc01ffaa6de1a2", urls = [ - "https://github.com/bazelbuild/bazel-gazelle/archive/622d8889c63227a71d6b393ba5e3a8b8a6761466.tar.gz", - ], - patch_args = ["-p1"], - patches = [ - # While we wait for https://github.com/bazelbuild/bazel-gazelle/pull/1324 to merge. - "@contrib_rules_jvm//java/gazelle/private/patches:bazel_gazelle-1324-allow-configuring-timeout-of-generation-tests.patch", + "https://github.com/bazelbuild/bazel-gazelle/archive/05f5493ca9b74f037dde928721dc01ffaa6de1a2.tar.gz", ], )