Fix Starlark CPU profiler resource leaks in Main and BlazeCommandDispatcher#29558
Open
SebTardif wants to merge 1 commit into
Open
Fix Starlark CPU profiler resource leaks in Main and BlazeCommandDispatcher#29558SebTardif wants to merge 1 commit into
SebTardif wants to merge 1 commit into
Conversation
…atcher Fix two resource management bugs in the Starlark CPU profiling lifecycle: 1. In Main.java (-cpuprofile flag): The output stream was opened before checking whether profiling startup succeeded, and stopCpuProfile() was called unconditionally even when startCpuProfile() returned false or the native support was unavailable. This could trigger a 'stop without start' error. Refactored to track startup success explicitly and only stop/close on the appropriate paths. 2. In BlazeCommandDispatcher.java (--starlark_cpu_profile flag): The output stream created for the profile was leaked on several early return paths (e.g. IllegalStateException from startCpuProfile, early exit from option parsing errors, or uncaught exceptions). Added proper cleanup in the finally block and on the init-failure path, and ensured stopCpuProfile() is not called twice if it throws IOException. Also extracted Main logic into a package-local run() method to make it testable without System.exit(). Tests: - MainTest: verifies -cpuprofile does not fail when profiling is unsupported (native support is null). - BlazeCommandDispatcherTest: verifies output stream is closed when profiling initialization fails (startTimer returns false) and when the command exits early after successful profiling start. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
55e5c39 to
b5f3510
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two resource management bugs exist in the Starlark CPU profiling lifecycle:
1.
Main.java(-cpuprofileflag)The output stream is opened before checking whether profiling startup succeeded.
stopCpuProfile()is called unconditionally even whenstartCpuProfile()returnsfalse(native support unavailable), which can trigger astop without starterror. The output stream is also leaked on failure.2.
BlazeCommandDispatcher.java(--starlark_cpu_profileflag)The output stream created for the profile is leaked on several early return paths:
IllegalStateExceptionfromstartCpuProfile()(e.g. SIGPROF in use)beforeCommandfailuresfinallyblockAdditionally, if
stopCpuProfile()throwsIOExceptionin the normal finalization path, thefinallyblock could attempt a second stop, retriggering astop without starterror.Fix
Main.java:
startCpuProfile()actually succeededstopCpuProfile()when it did; otherwise close the output stream directlyrun()method so the logic is testable withoutSystem.exit()BlazeCommandDispatcher.java:
startCpuProfile()exception pathfinallyblock: stop profiling if started, or close the stream if notIOExceptioncloseStarlarkCpuProfileOutput()for safe stream cleanupTests
MainTest(src/test/java/net/starlark/java/cmd/MainTest.java): Verifies-cpuprofiledoes not fail when profiling is unsupported (native support set to null).BlazeCommandDispatcherTest: Two new methods:starlarkCpuProfileOutputIsClosedWhenProfilingInitializationFails: Uses a fake native support wherestartTimer()returns false; verifies the output stream is closed and the correct failure code is returned.starlarkCpuProfileOutputIsClosedOnEarlyReturnAfterStart: Uses a fake native support wherestartTimer()succeeds; triggers an early exit via--config=UNDEFINED_CONFIG_VALUE; verifies the output stream is closed.