diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcher.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcher.kt index d3bbe9bebd..2327e3b27b 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcher.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcher.kt @@ -124,8 +124,34 @@ internal class GradleDaemonWatcher( .onFailure { err -> log.warn("Failed to report exit of Gradle daemon {}", pid, err) } } + /** + * Stops the poller. + * + * Graceful first, so a report already queued -- an exit picked up moments before the server was + * told to stop -- still runs; then forcefully, so a poll asleep between attempts cannot hold the + * process open. [SHUTDOWN_GRACE_MS] is the bound: work is one `ProcessHandle.descendants()` + * scan, not a build, so a poll that has not finished in that long is wedged rather than busy. + * + * A report that has *not* been submitted yet is lost, and that is accepted: it arrives via + * onExit on a process-reaper thread once the OS reaps the daemon, which at shutdown is after + * everything here has run. + */ fun shutdown() { - scheduler.shutdownNow() + scheduler.shutdown() + val drained = + runCatching { scheduler.awaitTermination(SHUTDOWN_GRACE_MS, TimeUnit.MILLISECONDS) } + .onFailure { err -> + // Only an interrupt is restored. Anything else out of awaitTermination says + // nothing about this thread's cancellation state, and marking it interrupted + // would abort the caller's next blocking call -- ToolingApiServerImpl.shutdown's + // wait on the connection close -- over a failure unrelated to it. + if (err is InterruptedException) { + Thread.currentThread().interrupt() + } + }.getOrDefault(false) + if (!drained) { + scheduler.shutdownNow() + } } companion object { @@ -143,6 +169,9 @@ internal class GradleDaemonWatcher( private const val POLL_INTERVAL_MS = 500L + /** How long [shutdown] lets queued reports finish before it stops waiting. */ + const val SHUTDOWN_GRACE_MS = 250L + /** Bounded at roughly a minute, which is far longer than a daemon takes to come up. */ const val MAX_POLL_ATTEMPTS = 120 diff --git a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt index dd5e0b8c26..ccc98ef720 100644 --- a/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt +++ b/subprojects/tooling-api-impl/src/main/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImpl.kt @@ -73,7 +73,10 @@ import kotlin.concurrent.withLock * * @author Akash Yadav */ -internal class ToolingApiServerImpl : IToolingApiServer { +internal class ToolingApiServerImpl( + private val newDaemonWatcher: (onStarted: (Int) -> Unit, onExited: (Int) -> Unit) -> GradleDaemonWatcher = + { onStarted, onExited -> GradleDaemonWatcher(onStarted = onStarted, onExited = onExited) }, +) : IToolingApiServer { private var client: IToolingApiClient? = null private var connector: GradleConnector? = null private var connection: ProjectConnection? = null @@ -361,11 +364,45 @@ internal class ToolingApiServerImpl : IToolingApiServer { * Finds the Gradle daemon and reports it to the client, so the memory chart can plot the process * that actually holds the build's heap (ADFA-5514). */ - private val daemonWatcher by lazy { - GradleDaemonWatcher( - onStarted = { pid -> client?.onGradleDaemonStarted(pid) }, - onExited = { pid -> client?.onGradleDaemonExited(pid) }, - ) + private val lazyDaemonWatcher = + lazy { + newDaemonWatcher( + { pid -> client?.onGradleDaemonStarted(pid) }, + { pid -> client?.onGradleDaemonExited(pid) }, + ) + } + + private val daemonWatcher by lazyDaemonWatcher + + /** + * Serialises constructing the watcher against stopping it. + * + * `shutdown` and `executeTasks` arrive as separate requests and run their bodies on the common + * pool, so a build submitted just before a shutdown can reach [startDaemonWatch] after shutdown + * has already asked whether a watcher exists. Without this, that build constructs a watcher, and + * its scheduler, that nothing will ever stop -- the leak this class's shutdown call exists to + * prevent, arriving by the one route the `isInitialized` check cannot see. + */ + private val daemonWatcherLock = Any() + + /** Guarded by [daemonWatcherLock]. */ + private var isDaemonWatcherShutdown = false + + /** + * Starts a daemon search for a build that is beginning, unless the server is shutting down. + * + * Only the construction is locked. `onBuildStarted` runs outside it, so a watcher stopped in + * between hits the rejection its own guard already handles rather than blocking a build thread. + */ + private fun startDaemonWatch() { + val watcher = + synchronized(daemonWatcherLock) { + if (isDaemonWatcherShutdown) { + return + } + daemonWatcher + } + watcher.onBuildStarted() } private fun notifyBuildFailure(result: BuildResult) { @@ -407,6 +444,37 @@ internal class ToolingApiServerImpl : IToolingApiServer { buildCancellationToken?.cancel() buildCancellationToken = null + // Early, and deliberately not "late enough to report the daemon's exit". + // + // The leaked thread is the defect: unstopped, an in-flight poll chain goes on scanning + // ProcessHandle.descendants() for up to a minute after the server is gone. Stopping it + // here ends that at once, and means no later poll can report a daemon into an RPC + // channel that is being torn down. + // + // Delivering the shutdown-time exit was tried and does not work. That report arrives + // through handle.onExit().thenRun { scheduler.execute { ... } }, and onExit completes + // on a process-reaper thread only once the OS has reaped the daemon -- strictly after + // DefaultGradleConnector.close() returns. There is no point in this sequence where the + // scheduler is still accepting work *and* the daemon has already been reaped, so the + // report is not deliverable at shutdown whatever the ordering; keeping `client` alive + // for it only widens the window in which a half-torn-down channel can be written to. + // The client learns the daemon is gone when it reconnects, not from here. + // + // Through the lazy delegate rather than the property: touching the property would + // construct a watcher, and its scheduler, only to shut it down again on a server that + // never ran a build. The flag closes the other half of that: a build that reaches + // startDaemonWatch after this point must not build one either. See daemonWatcherLock. + val watcher = + synchronized(daemonWatcherLock) { + isDaemonWatcherShutdown = true + if (lazyDaemonWatcher.isInitialized()) daemonWatcher else null + } + if (watcher != null) { + log.info("Stopping the Gradle daemon watcher...") + runCatching { watcher.shutdown() } + .onFailure { log.warn("Could not stop the Gradle daemon watcher", it) } + } + val connection = this.connection val connector = this.connector this.connection = null @@ -468,7 +536,7 @@ internal class ToolingApiServerImpl : IToolingApiServer { } isBuildInProgress = true - daemonWatcher.onBuildStarted() + startDaemonWatch() try { action() } finally { diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcherTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcherTest.kt index 2b6ac12aa5..1c45307d2e 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcherTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/GradleDaemonWatcherTest.kt @@ -202,6 +202,34 @@ class GradleDaemonWatcherTest { } } + @Test + fun `a wait that fails for another reason does not mark the thread interrupted`() { + // shutdown() runs as a teardown step on a shared pool worker, and its caller's next act is + // a blocking wait on the connection close. A flag set for a failure that has nothing to do + // with cancellation aborts that wait, so the connector teardown is never waited on. + val scheduler = mockk(relaxed = true) + every { scheduler.awaitTermination(any(), any()) } throws IllegalStateException("not an interrupt") + + watcher(scheduler = scheduler).shutdown() + + // Reads and clears, so a stray flag cannot leak into the tests that follow either. + assertThat(Thread.interrupted()).isFalse() + // The failure still counts as "not drained", so the forceful stop still runs. + verify(exactly = 1) { scheduler.shutdownNow() } + } + + @Test + fun `an interrupted wait still marks the thread interrupted`() { + // The other side of the narrowing: a real interrupt is a cancellation request and is not + // this class's to swallow. + val scheduler = mockk(relaxed = true) + every { scheduler.awaitTermination(any(), any()) } throws InterruptedException() + + watcher(scheduler = scheduler).shutdown() + + assertThat(Thread.interrupted()).isTrue() + } + private companion object { /** What the daemon's command line looks like on device, trimmed to the identifying part. */ const val DAEMON_COMMAND_LINE = @@ -213,4 +241,28 @@ class GradleDaemonWatcherTest { /** Every poll attempt, plus the initial schedule. */ const val MAX_SCHEDULES = GradleDaemonWatcher.MAX_POLL_ATTEMPTS + 1 } + + @Test + fun `shutdown drains what is queued before it stops waiting`() { + // Graceful first: a report queued moments before the server was told to stop still runs. + val scheduler = mockk(relaxed = true) + every { scheduler.awaitTermination(any(), any()) } returns true + + watcher(scheduler = scheduler).shutdown() + + verify(exactly = 1) { scheduler.shutdown() } + verify(exactly = 1) { scheduler.awaitTermination(GradleDaemonWatcher.SHUTDOWN_GRACE_MS, TimeUnit.MILLISECONDS) } + verify(exactly = 0) { scheduler.shutdownNow() } + } + + @Test + fun `shutdown stops waiting on a poll that will not finish`() { + // And forcefully after the grace period, so a wedged scan cannot hold the process open. + val scheduler = mockk(relaxed = true) + every { scheduler.awaitTermination(any(), any()) } returns false + + watcher(scheduler = scheduler).shutdown() + + verify(exactly = 1) { scheduler.shutdownNow() } + } } diff --git a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt index d0b127de24..782ae73ea5 100644 --- a/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt +++ b/subprojects/tooling-api-impl/src/test/java/com/itsaky/androidide/tooling/impl/ToolingApiServerImplTest.kt @@ -15,11 +15,14 @@ import io.mockk.spyk import io.mockk.verify import org.gradle.tooling.GradleConnector import org.gradle.tooling.ProjectConnection +import org.junit.Assert.fail import org.junit.Test import org.junit.runner.RunWith import org.junit.runners.JUnit4 import java.io.File +import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit +import java.util.concurrent.TimeoutException /** * @author Akash Yadav @@ -124,4 +127,97 @@ class ToolingApiServerImplTest { RootModelBuilder.build(initParams, any()) } } + + @Test + fun `shutting the server down stops the daemon watcher`() { + // The defect this PR exists to fix, pinned at the caller. The watcher's own shutdown() was + // already correct on stage -- what was missing was anything calling it, so a test of + // GradleDaemonWatcher.shutdown() in isolation passes against the unfixed server and pins + // nothing. Deleting the block in ToolingApiServerImpl.shutdown() has to fail a test. + val watcher = mockk(relaxed = true) + val server = ToolingApiServerImpl(newDaemonWatcher = { _, _ -> watcher }) + + // A build, to bring the watcher into being the way a session does: runBuild calls + // onBuildStarted, which is what initializes the lazy. + server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + server.shutdown().get(5, TimeUnit.SECONDS) + + verify(exactly = 1) { watcher.shutdown() } + } + + @Test + fun `a server that never ran a build does not build a watcher just to stop it`() { + // Through the lazy delegate, not the property: touching the property would construct a + // watcher, and its scheduler thread, only to shut it down again. + var built = 0 + val server = + ToolingApiServerImpl( + newDaemonWatcher = { _, _ -> + built++ + mockk(relaxed = true) + }, + ) + + server.shutdown().get(5, TimeUnit.SECONDS) + + assertThat(built).isEqualTo(0) + } + + @Test + fun `a build that starts after shutdown does not build a watcher`() { + // Half of the shutdown-versus-build race. A build request already in flight runs its body on + // the common pool, so it can reach startDaemonWatch after shutdown has looked for a watcher + // and found none. Building one here leaves a scheduler nothing will ever stop. + var built = 0 + val server = + ToolingApiServerImpl( + newDaemonWatcher = { _, _ -> + built++ + mockk(relaxed = true) + }, + ) + + server.shutdown().get(5, TimeUnit.SECONDS) + server.initialize(testInitParams()).get(5, TimeUnit.SECONDS) + + assertThat(built).isEqualTo(0) + } + + @Test + fun `shutdown waits for a watcher a concurrent build is building`() { + // The other half: the build gets there first, and is still inside the constructor when + // shutdown asks. `lazy.isInitialized()` reads false throughout that window, so without the + // lock shutdown concludes there is no watcher and returns while one is being built. + // + // The negative assertion is bounded rather than exact: it proves shutdown did not conclude + // within a second, against an unlocked shutdown that runs in milliseconds. + val watcher = mockk(relaxed = true) + val constructing = CountDownLatch(1) + val release = CountDownLatch(1) + val server = + ToolingApiServerImpl( + newDaemonWatcher = { _, _ -> + constructing.countDown() + assertThat(release.await(5, TimeUnit.SECONDS)).isTrue() + watcher + }, + ) + + val build = server.initialize(testInitParams()) + assertThat(constructing.await(5, TimeUnit.SECONDS)).isTrue() + + val shutdown = server.shutdown() + try { + shutdown.get(1, TimeUnit.SECONDS) + fail("shutdown completed while the watcher was still being constructed") + } catch (_: TimeoutException) { + // expected: shutdown is waiting on the lock the build holds + } + + release.countDown() + build.get(5, TimeUnit.SECONDS) + shutdown.get(5, TimeUnit.SECONDS) + + verify(exactly = 1) { watcher.shutdown() } + } }