Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support shutdown hooks with signals #3821

Merged
merged 6 commits into from
Mar 24, 2024
Merged

Support shutdown hooks with signals #3821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
85ddaa4
Support shutdown hooks with signals
ekrich Mar 8, 2024
1e6d445
Revert some changes
ekrich Mar 9, 2024
4d33a60
Avoid atexit when using signal termination
ekrich Mar 9, 2024
e3f75f1
Exit with the proper value
ekrich Mar 9, 2024
b67a679
Update test and rename object for spelling
ekrich Mar 18, 2024
c0c2870
Add comment and rename function
ekrich Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 24 additions & 6 deletions javalib/src/main/scala/java/lang/Runtime.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package java.lang
import java.io.File
import java.util.{Set => juSet}
import java.util.Comparator
import scala.scalanative.libc.signal
import scala.scalanative.libc.stdlib
import scala.scalanative.posix.unistd._
import scala.scalanative.windows.SysInfoApi._
Expand All @@ -18,6 +19,20 @@ class Runtime private () {
lazy val setupAtExitHandler = {
stdlib.atexit(() => Runtime.getRuntime().runHooks())
}

// https://docs.oracle.com/en/java/javase/21/docs/specs/man/java.html
// Currently, we use C lib signals so SIGHUP is not covered for POSIX platforms.

lazy val setupSignalHandler = {
signal.signal(signal.SIGINT, handleSignal(_))
signal.signal(signal.SIGTERM, handleSignal(_))
}
Comment on lines +26 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lazy val setupSignalHandler = {
signal.signal(signal.SIGINT, handleSignal(_))
signal.signal(signal.SIGTERM, handleSignal(_))
}
lazy val setupSignalHandler = {
signal.signal(signal.SIGHUP, handleSignal(_))
signal.signal(signal.SIGINT, handleSignal(_))
signal.signal(signal.SIGQUIT, handleSignal(_))
signal.signal(signal.SIGTERM, handleSignal(_))
}

let's add a few more expected reason to quit

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand "The JVM runs shutdown hooks only in case of normal terminations." - what this seems to mean is Ctrl-C SIGINT and SIGTERM. This is also in the current Test comments.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JVM catches signals to implement shutdown hooks for unexpected termination. The JVM uses SIGHUP, SIGINT, and SIGTERM to initiate the running of shutdown hooks.
Applications embedding the JVM frequently need to trap signals such as SIGINT or SIGTERM, which can lead to interference with the JVM signal handlers. The -Xrs option is available to address this issue. When -Xrs is used, the signal masks for SIGINT, SIGTERM, SIGHUP, and SIGQUIT aren't changed by the JVM, and signal handlers for these signals aren't installed.

https://docs.oracle.com/en/java/javase/21/docs/specs/man/java.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll add SIGHUP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, we need to try and get this to work properly this way first as we are using clib and SIGHUP is not a member. I second pass could use posixlib and something special for Windows to make it JVM compliant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind that SIGHUP is a signal which is send when SSH connection is hand up due network issue.

Without it, shutdown hook at CLI utilite won't work on this case and it's show stoper for real life use case I guess.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem but we have nothing now and to make it cross platform is more complex so it might be better to have something now.


private def handleSignal(sig: CInt): Unit = {
Runtime.getRuntime().runHooks()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it won't work when we'll have multiple threads running. Starting runHooks would also start scala.scanative.runtime.JoinNonDaemonThreads which would wait for all thread to finish. We should at least remove this shutdown hook.
Also signal handler might be run by any of the threads. Which might affect its execution.

exit(128 + sig)
}

private def ensureCanModify(hook: Thread): Unit = if (shutdownStarted) {
throw new IllegalStateException(
s"Shutdown sequence started, cannot add/remove hook $hook"
Expand All @@ -28,6 +43,7 @@ class Runtime private () {
ensureCanModify(thread)
hooks.add(thread)
setupAtExitHandler
setupSignalHandler
}

def removeShutdownHook(thread: Thread): Boolean = hooks.synchronized {
Expand All @@ -41,7 +57,7 @@ class Runtime private () {
.asInstanceOf[Array[Thread]]
.sorted(Ordering.by[Thread, Int](-_.getPriority()))
hooks.foreach { t =>
t.setUncaughtExceptionHandler(ShutdownHookUncoughExceptionHandler)
t.setUncaughtExceptionHandler(ShutdownHookUncaughtExceptionHandler)
}
// JDK specifies that hooks might run in any order.
// However, for Scala Native it might be beneficial to support partial ordering
Expand Down Expand Up @@ -70,16 +86,18 @@ class Runtime private () {
try t.run()
catch {
case ex: Throwable =>
ShutdownHookUncoughExceptionHandler.uncaughtException(t, ex)
ShutdownHookUncaughtExceptionHandler.uncaughtException(t, ex)
}
}
}
private def runHooks() = {
import scala.scalanative.meta.LinktimeInfo.isMultithreadingEnabled
hooks.synchronized {
shutdownStarted = true
if (isMultithreadingEnabled) runHooksConcurrent()
else runHooksSequential()
if (!shutdownStarted) {
shutdownStarted = true
if (isMultithreadingEnabled) runHooksConcurrent()
else runHooksSequential()
}
}
}

Expand Down Expand Up @@ -108,7 +126,7 @@ class Runtime private () {
exec(Array(cmd), envp, dir)
}

private object ShutdownHookUncoughExceptionHandler
private object ShutdownHookUncaughtExceptionHandler
extends Thread.UncaughtExceptionHandler {
def uncaughtException(t: Thread, e: Throwable): Unit = {
System.err.println(s"Shutdown hook $t failed, reason: $e")
Expand Down
4 changes: 2 additions & 2 deletions javalib/src/main/scala/scala/scalanative/runtime/JoinNonDeamonThreads.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ object JoinNonDaemonThreads {
try
Runtime.getRuntime().addShutdownHook {
val t = new Thread(() => {
def pollDaemonThreads = Registry.aliveThreads.iterator
def pollNonDaemonThreads = Registry.aliveThreads.iterator
.map(_.thread)
.filter { thread =>
thread != Thread.currentThread() && !thread.isDaemon() && thread
Expand All @@ -16,7 +16,7 @@ object JoinNonDaemonThreads {

Registry.onMainThreadTermination()
Iterator
.continually(pollDaemonThreads)
.continually(pollNonDaemonThreads)
.takeWhile(_.hasNext)
.flatten
.foreach(_.join())
Expand Down
2 changes: 1 addition & 1 deletion tools/src/test/scala/scala/scalanative/linker/MinimalRequiredSymbolsTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class MinimalRequiredSymbolsTest extends LinkerSpec {
@Test def multithreading(): Unit =
checkMinimalRequiredSymbols(withMultithreading = true)(expected =
if (isScala3) SymbolsCount(types = 1100, members = 6550)
else if (isScala2_13) SymbolsCount(types = 1050, members = 6600)
else if (isScala2_13) SymbolsCount(types = 1050, members = 6650)
else SymbolsCount(types = 1050, members = 7050)
)

Expand Down