From 265d630125db448ba0cdc3ab7e938beb50e93ed0 Mon Sep 17 00:00:00 2001 From: Erik Gahlin Date: Mon, 19 May 2025 13:38:38 +0000 Subject: [PATCH] 8357187: JFR: User-defined defaults should be respected when an incorrect setting is set Reviewed-by: mgronlun --- .../jdk/jfr/internal/EventControl.java | 8 +++--- .../jfr/internal/settings/CutoffSetting.java | 12 +++++--- .../jfr/internal/settings/PeriodSetting.java | 20 ++++++++++--- .../internal/settings/ThresholdSetting.java | 10 +++++-- .../internal/settings/ThrottleSetting.java | 23 +++++++++++---- .../classes/jdk/jfr/internal/util/Utils.java | 28 +++++++++++++++++++ 6 files changed, 81 insertions(+), 20 deletions(-) diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java index 19e2febf88e..50823951152 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java @@ -301,7 +301,7 @@ public final class EventControl { private static Control defineThreshold(PlatformEventType type) { String def = type.getAnnotationValue(Threshold.class, ThresholdSetting.DEFAULT_VALUE); type.add(PrivateAccess.getInstance().newSettingDescriptor(TYPE_THRESHOLD, Threshold.NAME, def, Collections.emptyList())); - return new Control(new ThresholdSetting(type), def); + return new Control(new ThresholdSetting(type, def), def); } private static Control defineStackTrace(PlatformEventType type) { @@ -313,13 +313,13 @@ public final class EventControl { private static Control defineCutoff(PlatformEventType type) { String def = type.getAnnotationValue(Cutoff.class, CutoffSetting.DEFAULT_VALUE); type.add(PrivateAccess.getInstance().newSettingDescriptor(TYPE_CUTOFF, Cutoff.NAME, def, Collections.emptyList())); - return new Control(new CutoffSetting(type), def); + return new Control(new CutoffSetting(type, def), def); } private static Control defineThrottle(PlatformEventType type) { String def = type.getAnnotationValue(Throttle.class, ThrottleSetting.DEFAULT_VALUE); type.add(PrivateAccess.getInstance().newSettingDescriptor(TYPE_THROTTLE, Throttle.NAME, def, Collections.emptyList())); - return new Control(new ThrottleSetting(type), def); + return new Control(new ThrottleSetting(type, def), def); } private static Control defineLevel(PlatformEventType type) { @@ -332,7 +332,7 @@ public final class EventControl { private static Control definePeriod(PlatformEventType type) { String def = type.getAnnotationValue(Period.class, PeriodSetting.DEFAULT_VALUE); type.add(PrivateAccess.getInstance().newSettingDescriptor(TYPE_PERIOD, PeriodSetting.NAME, def, Collections.emptyList())); - return new Control(new PeriodSetting(type), def); + return new Control(new PeriodSetting(type, def), def); } void disable() { diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/CutoffSetting.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/CutoffSetting.java index 2f7d9f8e872..1ba245babcc 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/CutoffSetting.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/CutoffSetting.java @@ -39,6 +39,7 @@ import jdk.jfr.Timespan; import jdk.jfr.internal.PlatformEventType; import jdk.jfr.internal.Type; import jdk.jfr.internal.util.ValueParser; +import jdk.jfr.internal.util.Utils; @MetadataDefinition @Label("Cutoff") @@ -47,11 +48,14 @@ import jdk.jfr.internal.util.ValueParser; @Timespan public final class CutoffSetting extends SettingControl { public static final String DEFAULT_VALUE = ValueParser.INFINITY; - private String value = DEFAULT_VALUE; private final PlatformEventType eventType; + private final String defaultValue; + private String value; - public CutoffSetting(PlatformEventType eventType) { - this.eventType = Objects.requireNonNull(eventType); + public CutoffSetting(PlatformEventType eventType, String defaultValue) { + this.eventType = Objects.requireNonNull(eventType); + this.defaultValue = Utils.validTimespanInfinity(eventType, "Cutoff", defaultValue, DEFAULT_VALUE); + this.value = defaultValue; } @Override @@ -65,7 +69,7 @@ public final class CutoffSetting extends SettingControl { max = nanos; } } - return Objects.requireNonNullElse(text, DEFAULT_VALUE); + return Objects.requireNonNullElse(text, defaultValue); } @Override diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/PeriodSetting.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/PeriodSetting.java index 902584a8819..33d79222617 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/PeriodSetting.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/PeriodSetting.java @@ -36,6 +36,8 @@ import jdk.jfr.SettingControl; import jdk.jfr.internal.PlatformEventType; import jdk.jfr.internal.Type; import jdk.jfr.internal.util.ValueParser; +import jdk.jfr.internal.util.Utils; + import static jdk.jfr.internal.util.ValueParser.MISSING; @MetadataDefinition @@ -44,17 +46,27 @@ import static jdk.jfr.internal.util.ValueParser.MISSING; @Name(Type.SETTINGS_PREFIX + "Period") public final class PeriodSetting extends SettingControl { private static final long typeId = Type.getTypeId(PeriodSetting.class); - public static final String EVERY_CHUNK = "everyChunk"; public static final String BEGIN_CHUNK = "beginChunk"; public static final String END_CHUNK = "endChunk"; public static final String DEFAULT_VALUE = EVERY_CHUNK; public static final String NAME = "period"; private final PlatformEventType eventType; - private String value = EVERY_CHUNK; + private final String defaultValue; + private String value; - public PeriodSetting(PlatformEventType eventType) { + public PeriodSetting(PlatformEventType eventType, String defaultValue) { this.eventType = Objects.requireNonNull(eventType); + this.defaultValue = validPeriod(defaultValue); + this.value = defaultValue; + } + + private String validPeriod(String userDefault) { + return switch (userDefault) { + case BEGIN_CHUNK -> BEGIN_CHUNK; + case END_CHUNK -> END_CHUNK; + default -> Utils.validTimespanInfinity(eventType, "Period", userDefault, DEFAULT_VALUE); + }; } @Override @@ -95,7 +107,7 @@ public final class PeriodSetting extends SettingControl { if (!beginChunk && endChunk) { return END_CHUNK; } - return DEFAULT_VALUE; // "everyChunk" is default + return defaultValue; } @Override diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThresholdSetting.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThresholdSetting.java index 83a55726707..348830938f4 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThresholdSetting.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThresholdSetting.java @@ -38,6 +38,7 @@ import jdk.jfr.SettingControl; import jdk.jfr.Timespan; import jdk.jfr.internal.PlatformEventType; import jdk.jfr.internal.Type; +import jdk.jfr.internal.util.Utils; import jdk.jfr.internal.util.ValueParser; @MetadataDefinition @@ -48,11 +49,14 @@ import jdk.jfr.internal.util.ValueParser; public final class ThresholdSetting extends SettingControl { public static final String DEFAULT_VALUE = "0 ns"; private static final long typeId = Type.getTypeId(ThresholdSetting.class); - private String value = DEFAULT_VALUE; private final PlatformEventType eventType; + private final String defaultValue; + private String value; - public ThresholdSetting(PlatformEventType eventType) { + public ThresholdSetting(PlatformEventType eventType, String defaultValue) { this.eventType = Objects.requireNonNull(eventType); + this.defaultValue = Utils.validTimespanInfinity(eventType, "Threshold", defaultValue, DEFAULT_VALUE); + this.value = defaultValue; } @Override @@ -68,7 +72,7 @@ public final class ThresholdSetting extends SettingControl { } } } - return Objects.requireNonNullElse(text, DEFAULT_VALUE); + return Objects.requireNonNullElse(text, defaultValue); } @Override diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThrottleSetting.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThrottleSetting.java index ab08d2fea1d..800443cfaed 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThrottleSetting.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThrottleSetting.java @@ -51,10 +51,24 @@ import jdk.jfr.internal.util.Utils; public final class ThrottleSetting extends SettingControl { public static final String DEFAULT_VALUE = Throttle.DEFAULT; private final PlatformEventType eventType; - private String value = DEFAULT_VALUE; + private final String defaultValue; + private String value; - public ThrottleSetting(PlatformEventType eventType) { - this.eventType = Objects.requireNonNull(eventType); + public ThrottleSetting(PlatformEventType eventType, String defaultValue) { + this.eventType = Objects.requireNonNull(eventType); + this.defaultValue = validRate(defaultValue); + this.value = defaultValue; + } + + private String validRate(String defaultValue) { + if (DEFAULT_VALUE.equals(defaultValue)) { + return DEFAULT_VALUE; // Fast path to avoid parsing + } + if (Rate.of(defaultValue) == null) { + Utils.warnInvalidAnnotation(eventType, "Throttle", defaultValue, DEFAULT_VALUE); + return DEFAULT_VALUE; + } + return defaultValue; } @Override @@ -70,8 +84,7 @@ public final class ThrottleSetting extends SettingControl { } } } - // "off" is default - return Objects.requireNonNullElse(text, DEFAULT_VALUE); + return Objects.requireNonNullElse(text, defaultValue); } @Override diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/util/Utils.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/util/Utils.java index 777725f88d4..7de6617f806 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/util/Utils.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/util/Utils.java @@ -55,6 +55,7 @@ import jdk.jfr.internal.LogLevel; import jdk.jfr.internal.LogTag; import jdk.jfr.internal.Logger; import jdk.jfr.internal.MirrorEvent; +import jdk.jfr.internal.PlatformEventType; import jdk.jfr.internal.SecuritySupport; import jdk.jfr.internal.Type; import jdk.jfr.internal.management.HiddenWait; @@ -459,4 +460,31 @@ public final class Utils { File file = subPath == null ? new File(path) : new File(path, subPath); return file.toPath().toAbsolutePath(); } + + + public static String validTimespanInfinity(PlatformEventType type, String annotation, String userDefault, String systemDefault) { + if (systemDefault.equals(userDefault)) { + return systemDefault; // Fast path to avoid parsing + } + if (ValueParser.parseTimespanWithInfinity(userDefault, ValueParser.MISSING) != ValueParser.MISSING) { + return userDefault; + } + warnInvalidAnnotation(type, annotation, userDefault, systemDefault); + return systemDefault; + } + + public static void warnInvalidAnnotation(PlatformEventType type, String annotation, String userDefault, String systemDefault) { + StringBuilder sb = new StringBuilder(); + sb.append("Programming error. Event setting "); + sb.append("@").append(annotation).append("(\"").append(userDefault).append("\")"); + sb.append(" is invalid on event "); + sb.append(type.getName()); + sb.append(", using "); + sb.append("@").append(annotation).append("(\"").append(systemDefault).append("\")"); + sb.append( " instead."); + if (type.isSystem()) { + throw new InternalError(sb.toString()); // Fail fast for JDK and JVM events + } + Logger.log(LogTag.JFR_SETTING, LogLevel.WARN, sb.toString()); + } }