ensure shutdown of the symlink planting pool

[benjaminp]

I was iteratively fixing some build failures with bazel and realized bazel was becoming more and more sluggish. Taking a thread dump, I observed way too many symlink planting threads:

$ grep -c 'Non-eager Symlink planter ' stacks.txt
36800

Try harder to ensure the symlink planting thread pool is released after the build is done.

[gemini-code-assist]

Code Review

This pull request addresses a resource leak where the symlink planting thread pool was not being shut down, leading to sluggishness in Bazel. The fix introduces a more robust shutdown mechanism by calling a new shutdown() method from a finally block, ensuring the thread pool is always released. The changes in ExecutionTool.java correctly plumb the IncrementalPackageRoots instance to the finalization logic. My review includes one high-severity comment on a potential race condition in the new shutdown() method in IncrementalPackageRoots.java.

[benjaminp]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses a resource leak by ensuring the symlink planting thread pool is shut down at the end of a build. The changes introduce a shutdown() method and call it from a finally block for robustness. My review focuses on consolidating all related cleanup logic into this new robust shutdown() method to prevent other potential memory leaks on build failure paths.

[gemini-code-assist]

This new shutdown() method is a great step towards preventing thread leaks. However, other cleanup logic in dropIntermediateStatesAndUnregisterFromEventBus() is only triggered on successful analysis, which can lead to memory leaks on failure paths since this IncrementalPackageRoots instance is retained across builds.

To make the cleanup more robust, I recommend consolidating all cleanup logic from dropIntermediateStatesAndUnregisterFromEventBus() into this shutdown() method, as it's called from a finally block in ExecutionTool. This includes unregistering from the event bus and clearing other state.

  public void shutdown() {
    if (eventBus != null) {
      try {
        eventBus.unregister(this);
      } catch (IllegalArgumentException e) {
        // Already unregistered, ignore.
      }
      eventBus = null;
    }

    synchronized (stateLock) {
      donePackages = null;
      lazilyPlantedSymlinks = null;
      maybeConflictingBaseNamesLowercase = ImmutableSet.of();
    }

    synchronized (symlinkPlantingPool) {
      if (!symlinkPlantingPool.isShutdown()) {
        ExecutorUtil.uninterruptibleShutdownNow(symlinkPlantingPool);
      }
    }
  }

[benjaminp]

I believe the assumption that IncrementalPackageRoots is retained across builds is not true.

[joeleba]

It is. Just that it's always replaced by another instance of IncrementalPackageRoots at the beginning of the next build. Feel free to treat that as out of scope for this PR though.

bazel/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java

Lines 271 to 285 in ca14334

	IncrementalPackageRoots incrementalPackageRoots =
	IncrementalPackageRoots.createAndRegisterToEventBus(
	getExecRoot(),
	// Single package path is a Skymeld prerequisite.
	Iterables.getOnlyElement(env.getPackageLocator().getPathEntries()),
	env.getEventBus(),
	env.getDirectories().getProductName() + "-",
	skyframeExecutor.getIgnoredPaths(),
	request.getOptions(BuildLanguageOptions.class).experimentalSiblingRepositoryLayout,
	runtime.getWorkspace().doesAllowExternalRepositories());
	incrementalPackageRoots.eagerlyPlantSymlinksToSingleSourceRoot();
	env.getSkyframeBuildView()
	.getArtifactFactory()
	.setPackageRoots(incrementalPackageRoots.getPackageRootLookup());

[benjaminp]

I suppose it is nicer for the exceptional and successful cleanup logic to be the same. So, I've updated the PR to put all logic in shutdown and also call that on analysis completion.

[fmeum]

@joeleba

[meisterT]

Change looks good to me, but I want Joe to also have a look when he is back next week

[joeleba]

LGTM. It's up to you whether you wanna address the suggestion by gemini.