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

RuntimeConfigDefault changes ignored on restart #19046

Merged
merged 1 commit into from
Sep 27, 2021
Merged

RuntimeConfigDefault changes ignored on restart #19046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 16 additions & 29 deletions ...ment/src/main/java/io/quarkus/deployment/configuration/RunTimeConfigurationGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ private RunTimeConfigurationGenerator() {
}

public static final class GenerateOperation implements AutoCloseable {
final boolean devMode;
final boolean liveReloadPossible;
final LaunchMode launchMode;
final AccessorFinder accessorFinder;
final ClassOutput classOutput;
Expand All @@ -291,8 +291,6 @@ public static final class GenerateOperation implements AutoCloseable {
// default values given in the build configuration
final Map<String, String> specifiedRunTimeDefaultValues;
final Map<String, String> buildTimeRunTimeVisibleValues;
// default values produced by extensions via build item
final Map<String, String> runTimeDefaults;
final Map<Container, MethodDescriptor> enclosingMemberMethods = new HashMap<>();
final Map<Class<?>, MethodDescriptor> groupInitMethods = new HashMap<>();
final Map<Class<?>, FieldDescriptor> configRootsByType = new HashMap<>();
Expand Down Expand Up @@ -325,7 +323,7 @@ public static final class GenerateOperation implements AutoCloseable {

GenerateOperation(Builder builder) {
this.launchMode = builder.launchMode;
this.devMode = builder.launchMode == LaunchMode.DEVELOPMENT;
this.liveReloadPossible = builder.liveReloadPossible;
final BuildTimeConfigurationReader.ReadResult buildTimeReadResult = builder.buildTimeReadResult;
buildTimeConfigResult = Assert.checkNotNullParam("buildTimeReadResult", buildTimeReadResult);
specifiedRunTimeDefaultValues = Assert.checkNotNullParam("specifiedRunTimeDefaultValues",
Expand All @@ -334,7 +332,6 @@ public static final class GenerateOperation implements AutoCloseable {
buildTimeReadResult.getBuildTimeRunTimeVisibleValues());
classOutput = Assert.checkNotNullParam("classOutput", builder.getClassOutput());
roots = Assert.checkNotNullParam("builder.roots", builder.getBuildTimeReadResult().getAllRoots());
runTimeDefaults = Assert.checkNotNullParam("runTimeDefaults", builder.getRunTimeDefaults());
additionalTypes = Assert.checkNotNullParam("additionalTypes", builder.getAdditionalTypes());
additionalBootstrapConfigSourceProviders = builder.getAdditionalBootstrapConfigSourceProviders();
staticConfigSources = builder.getStaticConfigSources();
Expand All @@ -351,7 +348,7 @@ public static final class GenerateOperation implements AutoCloseable {
mc.invokeSpecialMethod(MethodDescriptor.ofConstructor(Object.class), mc.getThis());
mc.returnValue(null);
}
if (devMode) {
if (liveReloadPossible) {
reinit = cc.getMethodCreator(REINIT);
reinit.setModifiers(Opcodes.ACC_STATIC | Opcodes.ACC_PUBLIC);
} else {
Expand Down Expand Up @@ -464,7 +461,7 @@ public void run() {
// make the build time config global until we read the run time config -
// at run time (when we're ready) we update the factory and then release the build time config
installConfiguration(clinitConfig, clinit);
if (devMode) {
if (liveReloadPossible) {
final ResultHandle buildTimeRunTimeDefaultValuesConfigSource = reinit
.readStaticField(C_BUILD_TIME_RUN_TIME_DEFAULTS_CONFIG_SOURCE);
// create the map for build time config source
Expand Down Expand Up @@ -544,7 +541,7 @@ public void run() {

// create the map for run time specified values config source
final ResultHandle specifiedRunTimeValues = clinit.newInstance(HM_NEW);
if (!devMode) {
if (!liveReloadPossible) {
//we don't need these in devmode
//including it would just cache the first values
//but these can already just be read directly, as we are in the same JVM
Expand All @@ -553,17 +550,11 @@ public void run() {
clinit.load(entry.getValue()));
}
}
for (Map.Entry<String, String> entry : runTimeDefaults.entrySet()) {
if (!specifiedRunTimeDefaultValues.containsKey(entry.getKey())) {
// only add entry if the user didn't override it
clinit.invokeVirtualMethod(HM_PUT, specifiedRunTimeValues, clinit.load(entry.getKey()),
clinit.load(entry.getValue()));
}
}
final ResultHandle specifiedRunTimeSource = clinit.newInstance(PCS_NEW, specifiedRunTimeValues,
clinit.load("Specified default values"), clinit.load(Integer.MIN_VALUE + 100));

cc.getFieldCreator(C_SPECIFIED_RUN_TIME_CONFIG_SOURCE)
.setModifiers(Opcodes.ACC_STATIC | (devMode ? Opcodes.ACC_VOLATILE : Opcodes.ACC_FINAL));
.setModifiers(Opcodes.ACC_STATIC | (liveReloadPossible ? Opcodes.ACC_VOLATILE : Opcodes.ACC_FINAL));
clinit.writeStaticField(C_SPECIFIED_RUN_TIME_CONFIG_SOURCE, specifiedRunTimeSource);

// add in the custom sources that bootstrap config needs
Expand Down Expand Up @@ -709,7 +700,7 @@ public void run() {
// config root field is volatile in dev mode, final otherwise; we initialize it from clinit, and readConfig in dev mode
cc.getFieldCreator(rootFieldDescriptor)
.setModifiers(Opcodes.ACC_PUBLIC | Opcodes.ACC_STATIC
| (devMode ? Opcodes.ACC_VOLATILE : Opcodes.ACC_FINAL));
| (liveReloadPossible ? Opcodes.ACC_VOLATILE : Opcodes.ACC_FINAL));

// construct instance in <clinit>
ResultHandle instance;
Expand All @@ -726,7 +717,7 @@ public void run() {
clinit.invokeVirtualMethod(SB_APPEND_STRING, clinitNameBuilder, clinit.load(root.getName()));
clinit.invokeStaticMethod(initGroup, clinitConfig, clinitNameBuilder, instance);
clinit.invokeVirtualMethod(SB_SET_LENGTH, clinitNameBuilder, clInitOldLen);
if (devMode) {
if (liveReloadPossible) {
instance = readConfig.readStaticField(rootFieldDescriptor);
readConfig.invokeVirtualMethod(SB_APPEND_STRING, readConfigNameBuilder,
readConfig.load(root.getName()));
Expand Down Expand Up @@ -785,7 +776,7 @@ public void run() {
clinit.invokeStaticMethod(CD_UNKNOWN_PROPERTIES,
clinit.readStaticField(FieldDescriptor.of(cc.getClassName(), "unused", List.class)));

if (devMode) {
if (liveReloadPossible) {
configSweepLoop(siParserBody, readConfig, runTimeConfig, getRegisteredRoots(RUN_TIME));
}
// generate sweep for run time
Expand Down Expand Up @@ -1672,10 +1663,10 @@ public static Builder builder() {
}

public static final class Builder {
public boolean liveReloadPossible;
private LaunchMode launchMode;
private ClassOutput classOutput;
private BuildTimeConfigurationReader.ReadResult buildTimeReadResult;
private Map<String, String> runTimeDefaults;
private List<Class<?>> additionalTypes;
private List<String> additionalBootstrapConfigSourceProviders;

Expand All @@ -1698,6 +1689,11 @@ public Builder setClassOutput(final ClassOutput classOutput) {
return this;
}

public Builder setLiveReloadPossible(boolean liveReloadPossible) {
this.liveReloadPossible = liveReloadPossible;
return this;
}

BuildTimeConfigurationReader.ReadResult getBuildTimeReadResult() {
return buildTimeReadResult;
}
Expand All @@ -1707,15 +1703,6 @@ public Builder setBuildTimeReadResult(final BuildTimeConfigurationReader.ReadRes
return this;
}

Map<String, String> getRunTimeDefaults() {
return runTimeDefaults;
}

public Builder setRunTimeDefaults(final Map<String, String> runTimeDefaults) {
this.runTimeDefaults = runTimeDefaults;
return this;
}

List<Class<?>> getAdditionalTypes() {
return additionalTypes;
}
Expand Down
37 changes: 26 additions & 11 deletions core/deployment/src/main/java/io/quarkus/deployment/steps/ConfigGenerationBuildStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static io.quarkus.deployment.util.ServiceUtil.classNamesNamedIn;
import static java.util.stream.Collectors.toList;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.lang.reflect.Modifier;
import java.util.ArrayList;
Expand All @@ -13,6 +14,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Properties;
import java.util.Set;
import java.util.stream.Collectors;

Expand All @@ -32,12 +34,14 @@
import io.quarkus.deployment.builditem.ConfigurationBuildItem;
import io.quarkus.deployment.builditem.ConfigurationTypeBuildItem;
import io.quarkus.deployment.builditem.GeneratedClassBuildItem;
import io.quarkus.deployment.builditem.GeneratedResourceBuildItem;
import io.quarkus.deployment.builditem.LaunchModeBuildItem;
import io.quarkus.deployment.builditem.LiveReloadBuildItem;
import io.quarkus.deployment.builditem.RunTimeConfigurationDefaultBuildItem;
import io.quarkus.deployment.builditem.StaticInitConfigSourceFactoryBuildItem;
import io.quarkus.deployment.builditem.StaticInitConfigSourceProviderBuildItem;
import io.quarkus.deployment.builditem.SuppressNonRuntimeConfigChangedWarningBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageResourceBuildItem;
import io.quarkus.deployment.builditem.nativeimage.ReflectiveClassBuildItem;
import io.quarkus.deployment.configuration.BuildTimeConfigurationReader;
import io.quarkus.deployment.configuration.RunTimeConfigurationGenerator;
Expand All @@ -46,15 +50,18 @@
import io.quarkus.deployment.logging.LoggingSetupBuildItem;
import io.quarkus.gizmo.ClassCreator;
import io.quarkus.gizmo.ClassOutput;
import io.quarkus.runtime.LaunchMode;
import io.quarkus.runtime.annotations.ConfigPhase;
import io.quarkus.runtime.annotations.StaticInitSafe;
import io.quarkus.runtime.configuration.ConfigChangeRecorder;
import io.quarkus.runtime.configuration.ConfigRecorder;
import io.quarkus.runtime.configuration.ConfigUtils;
import io.quarkus.runtime.configuration.ConfigurationRuntimeConfig;
import io.quarkus.runtime.configuration.RuntimeOverrideConfigSource;
import io.smallrye.config.ConfigSourceFactory;
import io.smallrye.config.PropertiesLocationConfigSourceFactory;

public class ConfigGenerationBuildStep {

@BuildStep
void deprecatedStaticInitBuildItem(
List<AdditionalStaticInitConfigSourceProviderBuildItem> additionalStaticInitConfigSourceProviders,
Expand All @@ -74,13 +81,27 @@ void staticInitSources(
PropertiesLocationConfigSourceFactory.class.getName()));
}

@BuildStep
GeneratedResourceBuildItem runtimeDefaultsConfig(List<RunTimeConfigurationDefaultBuildItem> runTimeDefaults,
BuildProducer<NativeImageResourceBuildItem> nativeImageResourceBuildItemBuildProducer)
throws IOException {
Properties p = new Properties();
for (var e : runTimeDefaults) {
p.setProperty(e.getKey(), e.getValue());
}
ByteArrayOutputStream out = new ByteArrayOutputStream();
p.store(out, null);
nativeImageResourceBuildItemBuildProducer
.produce(new NativeImageResourceBuildItem(ConfigUtils.QUARKUS_RUNTIME_CONFIG_DEFAULTS_PROPERTIES));
return new GeneratedResourceBuildItem(ConfigUtils.QUARKUS_RUNTIME_CONFIG_DEFAULTS_PROPERTIES, out.toByteArray());
}

/**
* Generate the Config class that instantiates MP Config and holds all the config objects
*/
@BuildStep
void generateConfigClass(
ConfigurationBuildItem configItem,
List<RunTimeConfigurationDefaultBuildItem> runTimeDefaults,
List<ConfigurationTypeBuildItem> typeItems,
LaunchModeBuildItem launchModeBuildItem,
BuildProducer<GeneratedClassBuildItem> generatedClass,
Expand All @@ -95,13 +116,6 @@ void generateConfigClass(
return;
}

Map<String, String> defaults = new HashMap<>();
for (RunTimeConfigurationDefaultBuildItem item : runTimeDefaults) {
if (defaults.putIfAbsent(item.getKey(), item.getValue()) != null) {
throw new IllegalStateException("More than one default value for " + item.getKey() + " was produced");
Copy link
Contributor

Choose a reason for hiding this comment

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

I have stumbled over this validation a few times in the past, since I tried to use this mechanism to set up default values for various config properties which then sometimes conflicted with properties set by other extensions. Specifically, I tried to set a default for the dev-services Docker image name this way.

It seems like this validation no longer exists now and that it may be a case of last build item wins. Meanwhile I found a different solution using a custom config source, but due to the indeterminism it may make sense to keep this validation in place somewhere. Thoughts?

}
}

Set<String> discoveredConfigSources = discoverService(ConfigSource.class, reflectiveClass);
Set<String> discoveredConfigSourceProviders = discoverService(ConfigSourceProvider.class, reflectiveClass);
Set<String> discoveredConfigSourceFactories = discoverService(ConfigSourceFactory.class, reflectiveClass);
Expand All @@ -120,7 +134,8 @@ void generateConfigClass(
.setBuildTimeReadResult(configItem.getReadResult())
.setClassOutput(new GeneratedClassGizmoAdaptor(generatedClass, false))
.setLaunchMode(launchModeBuildItem.getLaunchMode())
.setRunTimeDefaults(defaults)
.setLiveReloadPossible(launchModeBuildItem.getLaunchMode() == LaunchMode.DEVELOPMENT
|| launchModeBuildItem.isAuxiliaryApplication())
.setAdditionalTypes(typeItems.stream().map(ConfigurationTypeBuildItem::getValueType).collect(toList()))
.setAdditionalBootstrapConfigSourceProviders(
getAdditionalBootstrapConfigSourceProviders(additionalBootstrapConfigSourceProviders))
Expand Down Expand Up @@ -161,7 +176,7 @@ public void suppressNonRuntimeConfigChanged(
@BuildStep
@Record(ExecutionTime.RUNTIME_INIT)
public void checkForBuildTimeConfigChange(
ConfigChangeRecorder recorder, ConfigurationBuildItem configItem, LoggingSetupBuildItem loggingSetupBuildItem,
ConfigRecorder recorder, ConfigurationBuildItem configItem, LoggingSetupBuildItem loggingSetupBuildItem,
ConfigurationRuntimeConfig configurationConfig,
List<SuppressNonRuntimeConfigChangedWarningBuildItem> suppressNonRuntimeConfigChangedWarningItems) {
BuildTimeConfigurationReader.ReadResult readResult = configItem.getReadResult();
Expand Down
20 changes: 5 additions & 15 deletions core/deployment/src/main/java/io/quarkus/deployment/steps/DevServicesConfigBuildStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
import io.quarkus.deployment.builditem.DevServicesConfigResultBuildItem;
import io.quarkus.deployment.builditem.DevServicesLauncherConfigResultBuildItem;
import io.quarkus.deployment.builditem.DevServicesNativeConfigResultBuildItem;
import io.quarkus.deployment.builditem.LiveReloadBuildItem;
import io.quarkus.deployment.builditem.RunTimeConfigurationDefaultBuildItem;
import io.quarkus.deployment.builditem.ServiceStartBuildItem;

class DevServicesConfigBuildStep {
static volatile Map<String, String> oldConfig;

@BuildStep
List<DevServicesConfigResultBuildItem> deprecated(List<DevServicesNativeConfigResultBuildItem> items) {
Expand All @@ -30,18 +30,16 @@ List<DevServicesConfigResultBuildItem> deprecated(List<DevServicesNativeConfigRe
@BuildStep
@Produce(ServiceStartBuildItem.class)
DevServicesLauncherConfigResultBuildItem setup(BuildProducer<RunTimeConfigurationDefaultBuildItem> runtimeConfig,
List<DevServicesConfigResultBuildItem> devServicesConfigResultBuildItems,
LiveReloadBuildItem liveReloadBuildItem) {
List<DevServicesConfigResultBuildItem> devServicesConfigResultBuildItems) {
Map<String, String> newProperties = new HashMap<>(devServicesConfigResultBuildItems.stream().collect(
Collectors.toMap(DevServicesConfigResultBuildItem::getKey, DevServicesConfigResultBuildItem::getValue)));
Config config = ConfigProvider.getConfig();
PreviousConfig oldProperties = liveReloadBuildItem.getContextObject(PreviousConfig.class);
//check if there are existing already started dev services
//if there were no changes to the processors they don't produce config
//so we merge existing config from previous runs
//we also check the current config, as the dev service may have been disabled by explicit config
if (oldProperties != null) {
for (Map.Entry<String, String> entry : oldProperties.config.entrySet()) {
if (oldConfig != null) {
for (Map.Entry<String, String> entry : oldConfig.entrySet()) {
if (!newProperties.containsKey(entry.getKey())
&& config.getOptionalValue(entry.getKey(), String.class).isEmpty()) {
newProperties.put(entry.getKey(), entry.getValue());
Expand All @@ -51,15 +49,7 @@ DevServicesLauncherConfigResultBuildItem setup(BuildProducer<RunTimeConfiguratio
for (Map.Entry<String, String> entry : newProperties.entrySet()) {
runtimeConfig.produce(new RunTimeConfigurationDefaultBuildItem(entry.getKey(), entry.getValue()));
}
liveReloadBuildItem.setContextObject(PreviousConfig.class, new PreviousConfig(newProperties));
oldConfig = newProperties;
return new DevServicesLauncherConfigResultBuildItem(Collections.unmodifiableMap(newProperties));
}

static class PreviousConfig {
final Map<String, String> config;

public PreviousConfig(Map<String, String> config) {
this.config = config;
}
}
}
4 changes: 2 additions & 2 deletions ...e/configuration/ConfigChangeRecorder.java → ...runtime/configuration/ConfigRecorder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@
import io.quarkus.runtime.configuration.ConfigurationRuntimeConfig.BuildTimeMismatchAtRuntime;

@Recorder
public class ConfigChangeRecorder {
public class ConfigRecorder {

private static final Logger log = Logger.getLogger(ConfigChangeRecorder.class);
private static final Logger log = Logger.getLogger(ConfigRecorder.class);

public void handleConfigChange(ConfigurationRuntimeConfig configurationConfig, Map<String, String> buildTimeConfig) {
Config configProvider = ConfigProvider.getConfig();
Expand Down
Loading