This is an automated email from the ASF dual-hosted git repository.
hansva pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new 2f40c42157 [IT] hardening for selenium (#8237)
2f40c42157 is described below
commit 2f40c42157e30c94001f68adfe3d625ebe87f537
Author: Hans Van Akelyen <[email protected]>
AuthorDate: Wed Sep 2 14:11:24 2026 +0200
[IT] hardening for selenium (#8237)
---
.../hop/core/config/ConfigFileSerializer.java | 105 +++++++++----
.../java/org/apache/hop/core/config/HopConfig.java | 89 ++++++-----
.../apache/hop/core/config/plugin/ConfigFile.java | 25 ++-
.../hop/core/config/ConfigFileSerializerTest.java | 173 +++++++++++++++++++++
4 files changed, 326 insertions(+), 66 deletions(-)
diff --git
a/core/src/main/java/org/apache/hop/core/config/ConfigFileSerializer.java
b/core/src/main/java/org/apache/hop/core/config/ConfigFileSerializer.java
index 4c6d9c8cc7..15987135a1 100644
--- a/core/src/main/java/org/apache/hop/core/config/ConfigFileSerializer.java
+++ b/core/src/main/java/org/apache/hop/core/config/ConfigFileSerializer.java
@@ -24,8 +24,14 @@ import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.InvalidPathException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.nio.file.StandardCopyOption;
import java.util.HashMap;
import java.util.Map;
+import java.util.UUID;
import org.apache.commons.vfs2.FileObject;
import org.apache.hop.core.exception.HopException;
import org.apache.hop.core.json.HopJson;
@@ -42,40 +48,85 @@ public class ConfigFileSerializer implements
IHopConfigSerializer {
prettyPrinter.indentArraysWith(DefaultIndenter.SYSTEM_LINEFEED_INSTANCE);
String niceJson =
objectMapper.writer(prettyPrinter).writeValueAsString(configMap);
+ byte[] content = niceJson.getBytes(StandardCharsets.UTF_8);
- // Write to a new new file...
- //
- FileObject newFile = HopVfs.getFileObject(filename + ".new");
- if (newFile.exists() && !newFile.delete()) {
- throw new HopException("Unable to delete new config file " +
newFile.getName().getURI());
+ Path localFile = localPath(filename);
+ if (localFile == null) {
+ writeThroughVfs(filename, content);
+ } else {
+ writeAtomically(localFile, content);
}
+ } catch (Exception e) {
+ throw new HopException("Error writing to Hop configuration file : " +
filename, e);
+ }
+ }
- // Write to the new file (hop.config.new)
- //
- OutputStream outputStream = HopVfs.getOutputStream(newFile, false);
- outputStream.write(niceJson.getBytes(StandardCharsets.UTF_8));
- outputStream.close();
-
- // if this worked, delete the old file (hop.config.old)
- //
- FileObject oldFile = HopVfs.getFileObject(filename + ".old");
- if (oldFile.exists() && !oldFile.delete()) {
-
- throw new HopException("Unable to delete old config file " +
oldFile.getName().getURI());
+ /**
+ * Write the configuration next to where it belongs and move it into place
in one step.
+ *
+ * <p>The configuration used to be written to {@code <name>.new} and moved
over the real file,
+ * with the real file supposedly kept as {@code <name>.old} first. That
never happened: {@code
+ * canRenameTo} asks whether a rename is possible, it does not perform one,
so there was no backup
+ * - only a moment where the configuration existed under neither name. Two
writers made it worse,
+ * because both used those same two fixed names and tripped over each
other's temporary file,
+ * which is what Hop Web sessions saving at the same time ran into.
+ *
+ * <p>A temporary file of its own per write and a single move settle both:
nobody shares a
+ * temporary name, and a reader sees either the previous configuration or
the new one.
+ */
+ private void writeAtomically(Path file, byte[] content) throws Exception {
+ Path folder = file.toAbsolutePath().getParent();
+ if (folder != null) {
+ Files.createDirectories(folder);
+ }
+ // In the same folder as the file itself: a move is only atomic within one
file store.
+ Path temporary = Files.createTempFile(folder,
file.getFileName().toString() + ".", ".tmp");
+ try {
+ Files.write(temporary, content);
+ try {
+ Files.move(
+ temporary, file, StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE);
+ } catch (Exception atomicNotSupported) {
+ // Not every file store can do it. Still better than writing the file
in place.
+ Files.move(temporary, file, StandardCopyOption.REPLACE_EXISTING);
}
+ } finally {
+ // Only ever there when the move did not happen; leaving those behind
would fill the
+ // configuration folder with the leftovers of every failed save.
+ Files.deleteIfExists(temporary);
+ }
+ }
- // If this worked, rename the file to the old file (hop.config ->
hop.config.old)
- //
- FileObject file = HopVfs.getFileObject(filename);
- if (file.exists() && !file.canRenameTo(oldFile)) { // could be a new file
- throw new HopException("Unable to rename config file to .old : " +
file.getName().getURI());
+ /** The same, for a configuration that does not live on this machine. */
+ private void writeThroughVfs(String filename, byte[] content) throws
Exception {
+ FileObject temporary = HopVfs.getFileObject(filename + "." +
UUID.randomUUID() + ".tmp");
+ try {
+ try (OutputStream outputStream = HopVfs.getOutputStream(temporary,
false)) {
+ outputStream.write(content);
+ }
+ HopVfs.moveFile(temporary, HopVfs.getFileObject(filename));
+ } finally {
+ if (temporary.exists()) {
+ temporary.delete();
}
+ }
+ }
- // Now rename the new file to the final value...
- //
- HopVfs.moveFile(newFile, file);
- } catch (Exception e) {
- throw new HopException("Error writing to Hop configuration file : " +
filename, e);
+ /**
+ * The file as a local path, or null when it is somewhere only VFS can reach.
+ *
+ * <p>Decided on the name Hop was given rather than on what VFS makes of it:
a location like
+ * {@code s3://bucket/hop-config.json} is a perfectly valid relative path on
Linux, and would
+ * quietly be written to a folder called "s3:".
+ */
+ private static Path localPath(String filename) {
+ if (filename == null || filename.contains("://")) {
+ return null;
+ }
+ try {
+ return Paths.get(filename);
+ } catch (InvalidPathException e) {
+ return null;
}
}
diff --git a/core/src/main/java/org/apache/hop/core/config/HopConfig.java
b/core/src/main/java/org/apache/hop/core/config/HopConfig.java
index fba02b1b16..2c1d1146cf 100644
--- a/core/src/main/java/org/apache/hop/core/config/HopConfig.java
+++ b/core/src/main/java/org/apache/hop/core/config/HopConfig.java
@@ -59,23 +59,27 @@ public class HopConfig extends ConfigFile {
return instance;
}
- public synchronized void saveOption(String optionKey, Object optionValue) {
- try {
- HopConfig hopConfig = getInstance();
- hopConfig.configMap.put(optionKey, optionValue);
- saveToFile();
- } catch (Exception e) {
- throw new HopRuntimeException("Error saving configuration option '" +
optionKey + "'", e);
+ public void saveOption(String optionKey, Object optionValue) {
+ synchronized (CONFIG_LOCK) {
+ try {
+ HopConfig hopConfig = getInstance();
+ hopConfig.configMap.put(optionKey, optionValue);
+ saveToFile();
+ } catch (Exception e) {
+ throw new HopRuntimeException("Error saving configuration option '" +
optionKey + "'", e);
+ }
}
}
- public static synchronized void saveOptions(Map<String, Object>
extraOptions) {
- try {
- HopConfig hopConfig = getInstance();
- hopConfig.configMap.putAll(extraOptions);
- hopConfig.saveToFile();
- } catch (Exception e) {
- throw new HopRuntimeException("Error saving configuration options", e);
+ public static void saveOptions(Map<String, Object> extraOptions) {
+ synchronized (CONFIG_LOCK) {
+ try {
+ HopConfig hopConfig = getInstance();
+ hopConfig.configMap.putAll(extraOptions);
+ hopConfig.saveToFile();
+ } catch (Exception e) {
+ throw new HopRuntimeException("Error saving configuration options", e);
+ }
}
}
@@ -142,26 +146,35 @@ public class HopConfig extends ConfigFile {
}
}
+ /**
+ * The GUI properties, which are a map inside the map that gets written to
file.
+ *
+ * <p>Callers change the map they get back, so handing it out and writing
the configuration have
+ * to take turns: a change landing halfway through serialising it left
Jackson iterating a map
+ * that had moved under it.
+ */
public static Map<String, String> readGuiProperties() {
- try {
- Object propertiesObject =
getInstance().configMap.get(HOP_GUI_PROPERTIES_KEY);
- if (propertiesObject == null) {
- Map<String, String> map = new HashMap<>();
- getInstance().configMap.put(HOP_GUI_PROPERTIES_KEY, map);
- return map;
- } else if (propertiesObject instanceof Map) {
- @SuppressWarnings("unchecked")
- Map<String, String> propertiesMap = (Map<String, String>)
propertiesObject;
- return propertiesMap;
- } else {
- // If the object is not a Map, create a new one and log a warning
- System.err.println("Warning: GUI properties object is not a Map,
creating new one");
- Map<String, String> map = new HashMap<>();
- getInstance().configMap.put(HOP_GUI_PROPERTIES_KEY, map);
- return map;
+ synchronized (CONFIG_LOCK) {
+ try {
+ Object propertiesObject =
getInstance().configMap.get(HOP_GUI_PROPERTIES_KEY);
+ if (propertiesObject == null) {
+ Map<String, String> map = new HashMap<>();
+ getInstance().configMap.put(HOP_GUI_PROPERTIES_KEY, map);
+ return map;
+ } else if (propertiesObject instanceof Map) {
+ @SuppressWarnings("unchecked")
+ Map<String, String> propertiesMap = (Map<String, String>)
propertiesObject;
+ return propertiesMap;
+ } else {
+ // If the object is not a Map, create a new one and log a warning
+ System.err.println("Warning: GUI properties object is not a Map,
creating new one");
+ Map<String, String> map = new HashMap<>();
+ getInstance().configMap.put(HOP_GUI_PROPERTIES_KEY, map);
+ return map;
+ }
+ } catch (Exception e) {
+ throw new HopRuntimeException("Error getting GUI properties from the
Hop configuration", e);
}
- } catch (Exception e) {
- throw new HopRuntimeException("Error getting GUI properties from the Hop
configuration", e);
}
}
@@ -192,15 +205,21 @@ public class HopConfig extends ConfigFile {
}
public static void setGuiProperty(String key, String value) {
- readGuiProperties().put(key, value);
+ synchronized (CONFIG_LOCK) {
+ readGuiProperties().put(key, value);
+ }
}
public static String getGuiProperty(String key) {
- return readGuiProperties().get(key);
+ synchronized (CONFIG_LOCK) {
+ return readGuiProperties().get(key);
+ }
}
public static void setGuiProperties(Map<String, String> map) {
- readGuiProperties().putAll(map);
+ synchronized (CONFIG_LOCK) {
+ readGuiProperties().putAll(map);
+ }
}
public void reload() {
diff --git
a/core/src/main/java/org/apache/hop/core/config/plugin/ConfigFile.java
b/core/src/main/java/org/apache/hop/core/config/plugin/ConfigFile.java
index b42bd6963e..8562e1b32f 100644
--- a/core/src/main/java/org/apache/hop/core/config/plugin/ConfigFile.java
+++ b/core/src/main/java/org/apache/hop/core/config/plugin/ConfigFile.java
@@ -42,6 +42,21 @@ public abstract class ConfigFile implements IConfigFile {
public static final String HOP_VARIABLES_KEY = "variables";
public static final String HOP_CONFIG_KEY = "config";
+ /**
+ * Held while the configuration is written, and by the callers that change
what is written.
+ *
+ * <p>Hop Web serves many people from one JVM and they share this
configuration: two of them
+ * closing a dialog at the same moment had both writing the file at once,
which failed the save
+ * outright and reported it to whoever happened to be second. Shared by
every configuration file
+ * because there are only a handful of them and they are written rarely.
+ *
+ * <p>It does not cover a caller that changes the map it got from {@link
#getConfigMap()} without
+ * asking for it: {@link #getDescribedVariables()} is one, and it
deliberately stays out because
+ * it runs for every log message. What it does cover is the writing itself,
which is where the
+ * damage was.
+ */
+ protected static final Object CONFIG_LOCK = new Object();
+
@Getter
@Setter
@JsonProperty("config")
@@ -81,10 +96,12 @@ public abstract class ConfigFile implements IConfigFile {
}
public void saveToFile() throws HopException {
- try {
- serializer.writeToFile(getConfigFilename(), configMap);
- } catch (Exception e) {
- throw new HopException("Error saving configuration file '" +
getConfigFilename() + "'", e);
+ synchronized (CONFIG_LOCK) {
+ try {
+ serializer.writeToFile(getConfigFilename(), configMap);
+ } catch (Exception e) {
+ throw new HopException("Error saving configuration file '" +
getConfigFilename() + "'", e);
+ }
}
}
diff --git
a/core/src/test/java/org/apache/hop/core/config/ConfigFileSerializerTest.java
b/core/src/test/java/org/apache/hop/core/config/ConfigFileSerializerTest.java
new file mode 100644
index 0000000000..7cbfda289b
--- /dev/null
+++
b/core/src/test/java/org/apache/hop/core/config/ConfigFileSerializerTest.java
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hop.core.config;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Stream;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+/** How the Hop configuration file is written. */
+class ConfigFileSerializerTest {
+
+ @TempDir private Path folder;
+
+ private final ConfigFileSerializer serializer = new ConfigFileSerializer();
+
+ @Test
+ @DisplayName("what was written is what is read back")
+ void writesAndReadsTheConfiguration() throws Exception {
+ String filename = folder.resolve("hop-config.json").toString();
+
+ serializer.writeToFile(filename, Map.of("answer", "42"));
+
+ assertEquals("42", serializer.readFromFile(filename).get("answer"));
+ }
+
+ @Test
+ @DisplayName("writing again replaces the file, leaving nothing beside it")
+ void leavesNothingBehind() throws Exception {
+ String filename = folder.resolve("hop-config.json").toString();
+
+ serializer.writeToFile(filename, Map.of("answer", "42"));
+ serializer.writeToFile(filename, Map.of("answer", "43"));
+
+ assertEquals("43", serializer.readFromFile(filename).get("answer"));
+ assertEquals(List.of("hop-config.json"), filesInFolder());
+ }
+
+ /**
+ * Two Hop Web sessions closing a dialog at the same moment both save the
configuration. Writing
+ * through one fixed temporary name had them deleting and moving each
other's file, and whoever
+ * lost the race was told the save had failed - the failure this test exists
for.
+ */
+ @Test
+ @DisplayName("several writers at once all succeed")
+ void writesFromSeveralThreads() throws Exception {
+ String filename = folder.resolve("hop-config.json").toString();
+ int writers = 8;
+ int writesEach = 25;
+
+ CountDownLatch startTogether = new CountDownLatch(1);
+ CountDownLatch finished = new CountDownLatch(writers);
+ AtomicReference<Exception> failure = new AtomicReference<>();
+ List<Thread> threads = new ArrayList<>();
+
+ for (int writer = 0; writer < writers; writer++) {
+ String value = "writer-" + writer;
+ Thread thread =
+ new Thread(
+ () -> {
+ try {
+ startTogether.await();
+ for (int i = 0; i < writesEach; i++) {
+ serializer.writeToFile(filename, Map.of("written-by",
value));
+ }
+ } catch (Exception e) {
+ failure.compareAndSet(null, e);
+ } catch (Throwable e) {
+ failure.compareAndSet(null, new IllegalStateException(e));
+ } finally {
+ finished.countDown();
+ }
+ });
+ threads.add(thread);
+ thread.start();
+ }
+
+ startTogether.countDown();
+ assertTrue(finished.await(60, TimeUnit.SECONDS), "the writers did not
finish");
+ for (Thread thread : threads) {
+ thread.join();
+ }
+
+ if (failure.get() != null) {
+ throw new AssertionError("a writer failed to save the configuration",
failure.get());
+ }
+ // Whichever writer went last, the file is one of theirs and it is
complete.
+ assertNotNull(serializer.readFromFile(filename).get("written-by"));
+ assertEquals(List.of("hop-config.json"), filesInFolder());
+ }
+
+ /** A reader only ever sees a complete configuration, never a half written
one. */
+ @Test
+ @DisplayName("a reader never catches the file half written")
+ void isNeverReadHalfWritten() throws Exception {
+ String filename = folder.resolve("hop-config.json").toString();
+ Map<String, Object> big = new HashMap<>();
+ for (int i = 0; i < 500; i++) {
+ big.put("key-" + i, "value-" + i);
+ }
+ serializer.writeToFile(filename, big);
+
+ AtomicReference<Exception> failure = new AtomicReference<>();
+ CountDownLatch done = new CountDownLatch(1);
+ Thread writer =
+ new Thread(
+ () -> {
+ try {
+ for (int i = 0; i < 50; i++) {
+ serializer.writeToFile(filename, big);
+ }
+ } catch (Exception e) {
+ failure.compareAndSet(null, e);
+ } finally {
+ done.countDown();
+ }
+ });
+ writer.start();
+
+ while (done.getCount() > 0) {
+ // On Windows the file cannot always be opened while it is being
replaced; that a read
+ // failed is fine, that it succeeded and came back incomplete is not.
+ try {
+ Map<String, Object> read = serializer.readFromFile(filename);
+ if (!read.isEmpty()) {
+ assertEquals(big.size(), read.size(), "the configuration was read
half written");
+ }
+ } catch (Exception acceptable) {
+ // See above.
+ }
+ }
+ writer.join();
+
+ if (failure.get() != null) {
+ throw new AssertionError("the writer failed to save the configuration",
failure.get());
+ }
+ }
+
+ private List<String> filesInFolder() throws IOException {
+ try (Stream<Path> files = Files.list(folder)) {
+ return files.map(path ->
path.getFileName().toString()).sorted().toList();
+ }
+ }
+}
