From b50b13db15900bb84b1d76af183b87d0f1f2ccf8 Mon Sep 17 00:00:00 2001 From: Devin Smith Date: Fri, 5 Apr 2024 10:12:15 -0700 Subject: [PATCH] Remove DisableIntrinsic for currentThread (#5295) Provides a framework for adding Deephaven safety checks on startup. Currently, the only safety check is https://bugs.openjdk.org/browse/JDK-8287432. We could consider adding other safety checks; for example, throwing errors when we know a Java version is not longer supported. Fixes #2500 --- ...eephaven.java-toolchain-conventions.gradle | 8 +- .../deephaven_server/start_jvm.py | 2 - .../deephaven/server/runner/MainHelper.java | 3 + .../deephaven/server/runner/SafetyChecks.java | 124 ++++++++++++++++++ 4 files changed, 130 insertions(+), 7 deletions(-) create mode 100644 server/src/main/java/io/deephaven/server/runner/SafetyChecks.java diff --git a/buildSrc/src/main/groovy/io.deephaven.java-toolchain-conventions.gradle b/buildSrc/src/main/groovy/io.deephaven.java-toolchain-conventions.gradle index 2ae1733cd0a..82c6e56268d 100644 --- a/buildSrc/src/main/groovy/io.deephaven.java-toolchain-conventions.gradle +++ b/buildSrc/src/main/groovy/io.deephaven.java-toolchain-conventions.gradle @@ -94,9 +94,9 @@ def createCompilerDirectives = tasks.register('createCompilerDirectives') { def compilerDirectivesFile = project.layout.buildDirectory.file('dh-compiler-directives.txt') def compilerDirectivesText = new JsonBuilder([{ match (['*.*'] as List) - // Note: there seems to be a bug where this option doesn't actually get picked up - // So using '-XX:DisableIntrinsic=_currentThread' explicitly - // DisableIntrinsic('_currentThread') + // Note: historically, there seemed to be a bug in the JVMs where DisableIntrinsic('_currentThread') didn't actually + // get picked up from this source. Any new options added here should have sufficient due diligence to ensure they + // are being picked up as appropriate. }]).toPrettyString() it.inputs.property('compilerDirectivesText', compilerDirectivesText) it.outputs.file(compilerDirectivesFile) @@ -109,8 +109,6 @@ def compilerArgs = { String compilerDirectivesFile -> return [ '-XX:+UnlockDiagnosticVMOptions', "-XX:CompilerDirectivesFile=${compilerDirectivesFile}", - // (deephaven-core#2500): Remove DisableIntrinsic for currentThread - '-XX:DisableIntrinsic=_currentThread', // '-XX:+CompilerDirectivesPrint', // '-XX:+LogCompilation', ] diff --git a/py/embedded-server/deephaven_server/start_jvm.py b/py/embedded-server/deephaven_server/start_jvm.py index e07412e333b..3467997c82f 100644 --- a/py/embedded-server/deephaven_server/start_jvm.py +++ b/py/embedded-server/deephaven_server/start_jvm.py @@ -43,8 +43,6 @@ def _jars(): # Disable JIT in certain cases '-XX:+UnlockDiagnosticVMOptions', f"-XX:CompilerDirectivesFile={_compiler_directives()}", - # (deephaven-core#2500): Remove DisableIntrinsic for currentThread - '-XX:DisableIntrinsic=_currentThread', f"-XX:VMOptionsFile={_default_vmoptions()}", ] diff --git a/server/src/main/java/io/deephaven/server/runner/MainHelper.java b/server/src/main/java/io/deephaven/server/runner/MainHelper.java index dd3607ce56d..ae8ac4fda4b 100644 --- a/server/src/main/java/io/deephaven/server/runner/MainHelper.java +++ b/server/src/main/java/io/deephaven/server/runner/MainHelper.java @@ -107,6 +107,9 @@ public static Configuration init(String[] args, Class mainClass) throws IOExc // Capture the original System.out and System.err early PrintStreamGlobals.init(); + // Safety checks + SafetyChecks.check(); + // Since our dagger injection happens later, we need to provider a static way to get the LogBuffer (for example, // logback configuration may reference LogBufferAppender). LogBufferGlobal.setInstance(new LogBufferInterceptor(Integer.getInteger("logBuffer.history", 1024))); diff --git a/server/src/main/java/io/deephaven/server/runner/SafetyChecks.java b/server/src/main/java/io/deephaven/server/runner/SafetyChecks.java new file mode 100644 index 00000000000..9df39d2a2f2 --- /dev/null +++ b/server/src/main/java/io/deephaven/server/runner/SafetyChecks.java @@ -0,0 +1,124 @@ +// +// Copyright (c) 2016-2024 Deephaven Data Labs and Patent Pending +// +package io.deephaven.server.runner; + +import java.lang.Runtime.Version; +import java.lang.management.ManagementFactory; +import java.util.List; + +/** + * Deephaven safety checks. + */ +public final class SafetyChecks { + public static final String SAFETY_CHECKS_PROPERTY = "deephaven.safetyChecks"; + public static final String SAFETY_CHECKS_ENV = "DEEPHAVEN_SAFETY_CHECKS"; + + /** + * Checks whether safety checks are enabled. This parses the system property {@value SAFETY_CHECKS_PROPERTY} if it + * exists, otherwise it parses the environment variable {@value SAFETY_CHECKS_ENV} if it exists, otherwise defaults + * to {@code true}. Disabling safety checks is not recommended. + * + * @return if safety checks are enabled. + */ + public static boolean isEnabled() { + return parseBoolean(SAFETY_CHECKS_PROPERTY, SAFETY_CHECKS_ENV, true); + } + + /** + * Applies a set of safety checks if {@link #isEnabled()}. + * + * @see JDK_8287432 + */ + public static void check() { + if (!isEnabled()) { + return; + } + JDK_8287432.check(); + } + + private static boolean isEnabled(Class clazz) { + return parseBoolean(safetyCheckPropertyName(clazz), safetyCheckEnvironmentName(clazz), true); + } + + private static String safetyCheckEnvironmentName(Class clazz) { + return "DEEPHAVEN_SAFETY_CHECK_" + clazz.getSimpleName(); + } + + private static String safetyCheckPropertyName(Class clazz) { + return "deephaven.safetyCheck." + clazz.getSimpleName(); + } + + private static boolean parseBoolean(String propertyName, String envName, boolean defaultValue) { + final String propertyValue = System.getProperty(propertyName); + if (propertyValue != null) { + return Boolean.parseBoolean(propertyValue); + } + final String envValue = System.getenv(envName); + if (envValue != null) { + return Boolean.parseBoolean(envValue); + } + return defaultValue; + } + + private static String disableSafetyCheckMessage(Class clazz) { + return String.format( + "To disable this safety check (not recommended), you can set the system property `-D%s=false` or environment variable `%s=false`.", + safetyCheckPropertyName(clazz), safetyCheckEnvironmentName(clazz)); + } + + private static String disableSafetyChecksMessage() { + return String.format( + "To disable all safety checks (not recommended), you can set the system property `-D%s=false` or environment variable `%s=false`.", + SAFETY_CHECKS_PROPERTY, SAFETY_CHECKS_ENV); + } + + private static IllegalStateException exception(Class clazz, String baseMessage) { + return new IllegalStateException(String.format( + "%s %s %s", + baseMessage, + disableSafetyCheckMessage(clazz), + disableSafetyChecksMessage())); + } + + /** + * A safety check for JDK-8287432. + */ + public static final class JDK_8287432 { + + private static void check() { + if (!isEnabled(JDK_8287432.class)) { + return; + } + if (isVulnerableVersion() && !hasWorkaround()) { + throw exception(JDK_8287432.class, String.format( + "The current JDK is vulnerable to the bug https://bugs.openjdk.org/browse/JDK-8287432. We recommend updating to 11.0.17+, 17.0.5+, or 21+. If that is not possible, you can apply a workaround '%s'.", + String.join(" ", workaroundJvmArguments()))); + } + } + + private static boolean isVulnerableVersion(Version version) { + switch (version.feature()) { + case 11: + return version.compareTo(Version.parse("11.0.17")) < 0; + case 17: + return version.compareTo(Version.parse("17.0.5")) < 0; + } + // Assume the version does not have the bug. + return false; + } + + private static boolean isVulnerableVersion() { + return isVulnerableVersion(Runtime.version()); + } + + private static boolean hasWorkaround() { + return ManagementFactory.getRuntimeMXBean().getInputArguments() + .contains("-XX:DisableIntrinsic=_currentThread"); + } + + private static List workaroundJvmArguments() { + return List.of("-XX:+UnlockDiagnosticVMOptions", "-XX:DisableIntrinsic=_currentThread"); + } + } +}