Skip to content

Commit

Permalink
Remove DisableIntrinsic for currentThread (#5295)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
devinrsmith authored Apr 5, 2024
1 parent 664a1f5 commit b50b13d
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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',
]
Expand Down
2 changes: 0 additions & 2 deletions py/embedded-server/deephaven_server/start_jvm.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()}",
]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
Expand Down
124 changes: 124 additions & 0 deletions server/src/main/java/io/deephaven/server/runner/SafetyChecks.java
Original file line number Diff line number Diff line change
@@ -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 <a href="https://bugs.openjdk.org/browse/JDK-8287432">JDK-8287432</a>.
*/
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<String> workaroundJvmArguments() {
return List.of("-XX:+UnlockDiagnosticVMOptions", "-XX:DisableIntrinsic=_currentThread");
}
}
}

0 comments on commit b50b13d

Please sign in to comment.