diff --git a/main-command/src/main/scala/sbt/internal/server/Server.scala b/main-command/src/main/scala/sbt/internal/server/Server.scala index d7ee44284..e71d6e643 100644 --- a/main-command/src/main/scala/sbt/internal/server/Server.scala +++ b/main-command/src/main/scala/sbt/internal/server/Server.scala @@ -159,7 +159,6 @@ private[sbt] object Server { } override def shutdown(): Unit = { - log.info("shutting down sbt server") if (portfile.exists) { IO.delete(portfile) } @@ -171,6 +170,7 @@ private[sbt] object Server { case null => case s => s.close() } + log.info("shutting down sbt server") } private def writeTokenfile(): Unit = { diff --git a/main/src/main/scala/sbt/internal/CommandExchange.scala b/main/src/main/scala/sbt/internal/CommandExchange.scala index 3a5979e9f..8b2010c1f 100644 --- a/main/src/main/scala/sbt/internal/CommandExchange.scala +++ b/main/src/main/scala/sbt/internal/CommandExchange.scala @@ -320,9 +320,9 @@ private[sbt] final class CommandExchange { } procFile = None fastTrackThread.close() - channels foreach (_.shutdown(true)) + channels.foreach(c => Util.ignoreResult(Try(c.shutdown(true)))) // interrupt and kill the thread - server.foreach(_.shutdown()) + server.foreach(s => Util.ignoreResult(Try(s.shutdown()))) server = None EvaluateTask.onShutdown() } diff --git a/notes/2.0.0/reboot-reconnect.md b/notes/2.0.0/reboot-reconnect.md new file mode 100644 index 000000000..251be0527 --- /dev/null +++ b/notes/2.0.0/reboot-reconnect.md @@ -0,0 +1,15 @@ +### `reboot` works from the thin client again + +Running `reboot` in the sbt shell dropped to the OS shell ("sbt server connection +closed") instead of rebooting. The server's teardown began with a log line that +throws when the terminal in scope is the rebooting client's already-closed +virtual terminal, aborting teardown before the server socket was closed and the +portfile deleted; the relaunched instance then mistook the leaked socket for +another running sbt and never started its server, while the client latched onto +the stale portfile. Server teardown now completes its state cleanup before +logging, and one failing channel shutdown can no longer skip the rest of the +exchange teardown. `reboot` returns to a working prompt. + +This addresses [#9095][i9095]. + +[i9095]: https://github.com/sbt/sbt/issues/9095 diff --git a/server-test/src/test/scala/testpkg/AbstractServerTest.scala b/server-test/src/test/scala/testpkg/AbstractServerTest.scala index ca66b95b1..11f438db7 100644 --- a/server-test/src/test/scala/testpkg/AbstractServerTest.scala +++ b/server-test/src/test/scala/testpkg/AbstractServerTest.scala @@ -27,8 +27,15 @@ final class SbtServer( val baseDirectory: File, private val process: scala.sys.process.Process ) { - def close(): Unit = - session.shutdown(process.isAlive(), () => process.destroy()).get + def close(): Unit = { + val result = scala.util.Try(session.shutdown(process.isAlive(), () => process.destroy()).get) + if (process.isAlive()) process.destroy() + result match { + case scala.util.Failure(e) => + System.err.println(s"server session shutdown failed (process destroyed): $e") + case _ => + } + } } trait AbstractServerTest extends AnyFunSuite with BeforeAndAfterAll { diff --git a/server-test/src/test/scala/testpkg/RebootTest.scala b/server-test/src/test/scala/testpkg/RebootTest.scala new file mode 100644 index 000000000..23a7c60db --- /dev/null +++ b/server-test/src/test/scala/testpkg/RebootTest.scala @@ -0,0 +1,67 @@ +/* + * sbt + * Copyright 2023, Scala center + * Copyright 2011 - 2022, Lightbend, Inc. + * Copyright 2008 - 2010, Mark Harrah + * Licensed under Apache License 2.0 (see LICENSE) + */ + +package testpkg + +import java.io.{ InputStream, PrintStream } +import java.util.concurrent.{ LinkedBlockingQueue, TimeUnit, TimeoutException } +import sbt.internal.client.NetworkClient +import sbt.internal.util.Util + +/** + * Regression for https://github.com/sbt/sbt/issues/9095: `reboot` from a client must bring the + * server back and complete instead of leaving a zombie server that drops the client. + */ +class RebootTest extends AbstractServerTest { + override val testDirectory: String = "client" + + private object BlockingInputStream extends InputStream { + override def read(): Int = { + try Thread.sleep(Long.MaxValue) + catch { case _: InterruptedException => } + -1 + } + } + private val nullPrintStream = new PrintStream(_ => {}, false) + + private def background[R](f: => R): R = { + val result = new LinkedBlockingQueue[Either[Throwable, R]] + val thread = new Thread("reboot-test-client") { + setDaemon(true) + override def run(): Unit = + try Util.ignoreResult(result.put(Right(f))) + catch { case e: Throwable => Util.ignoreResult(result.put(Left(e))) } + } + thread.start() + result.poll(3, TimeUnit.MINUTES) match { + case null => + thread.interrupt() + thread.join(10000) + throw new TimeoutException("client did not complete within 3 minutes") + case Left(e) => throw e + case Right(r) => r + } + } + + private def client(args: String*): Int = + background( + NetworkClient.client( + testPath.toFile, + args.toArray, + BlockingInputStream, + nullPrintStream, + nullPrintStream, + false + ) + ) + + test("reboot completes and the rebooted server serves the next command") { + assert(client("reboot") == 0, "reboot from a client must complete with exit 0") + assert(client("willSucceed") == 0, "the rebooted server must serve a new client connection") + } +}