jbonofre commented on code in PR #3001: URL: https://github.com/apache/karaf/pull/3001#discussion_r4308567606
########## util/src/main/java/org/apache/karaf/util/FileUtils.java: ########## @@ -0,0 +1,61 @@ +/* + * 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.karaf.util; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.StandardCopyOption; + +public final class FileUtils { + + private FileUtils() { + } + + /** + * Writes a file through a temporary file next to it, which is moved over the file + * atomically once it is complete. The file therefore always holds either its previous + * or its new content, even if the process dies or the write fails part way through. + * A write that fails deletes the temporary file. + * + * @param file the file to write + * @param writer writes the new content to the stream it is given + * @throws IOException if the content cannot be written or moved over the file + */ + public static void atomicSave(File file, StreamWriter writer) throws IOException { + File tmpFile = new File(file.getPath() + ".tmp"); + try { + try (OutputStream out = new FileOutputStream(tmpFile)) { + writer.write(out); + } Review Comment: The temp file is moved into place without being forced to disk first. A process crash or a failed write is covered now, but after a power loss or an OS crash the rename can reach the disk before the data does (XFS, NTFS, ext4 with `noauto_da_alloc`). The result is an empty or zero-filled `digraph.json`, and the features service then fails to start. That's the case the Javadoc says can't happen. `JsonWriter.write` already flushes its writer, so syncing the descriptor before close is enough: ```suggestion try (FileOutputStream out = new FileOutputStream(tmpFile)) { writer.write(out); out.getFD().sync(); } ``` It would also help to say in the `StreamWriter` Javadoc that implementations must flush everything to the stream before returning. ########## features/core/src/main/java/org/apache/karaf/features/internal/region/DigraphHelper.java: ########## @@ -76,10 +76,9 @@ public static StandardRegionDigraph loadDigraph(BundleContext bundleContext) thr } public static void saveDigraph(File outFile, RegionDigraph digraph) { - try ( - FileOutputStream out = new FileOutputStream(outFile) - ) { - saveDigraph(digraph, out); + // A failure or a crash while saving never leaves a truncated digraph + try { + FileUtils.atomicSave(outFile, out -> saveDigraph(digraph, out)); } catch (Exception e) { // Ignore } Review Comment: Now that the save can also fail on the move, ignoring the exception means the digraph can quietly stop being saved while `state.json` keeps saving. The two files then drift apart with nothing in the log. Can we at least log a warning? ```suggestion } catch (Exception e) { LOGGER.warn("Error saving the region digraph to {}", outFile, e); } ``` (This needs a `private static final Logger LOGGER = LoggerFactory.getLogger(DigraphHelper.class);`, the same as `SubsystemResolver` has.) ########## util/src/main/java/org/apache/karaf/util/FileUtils.java: ########## @@ -0,0 +1,61 @@ +/* + * 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.karaf.util; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.StandardCopyOption; + +public final class FileUtils { + + private FileUtils() { + } + + /** + * Writes a file through a temporary file next to it, which is moved over the file + * atomically once it is complete. The file therefore always holds either its previous + * or its new content, even if the process dies or the write fails part way through. + * A write that fails deletes the temporary file. + * + * @param file the file to write + * @param writer writes the new content to the stream it is given + * @throws IOException if the content cannot be written or moved over the file + */ + public static void atomicSave(File file, StreamWriter writer) throws IOException { + File tmpFile = new File(file.getPath() + ".tmp"); Review Comment: With a fixed `.tmp` name, two saves of the same file running at once share one temp file. Digraph saves aren't serialized (`RegionDigraphPersistence.save()`, `Activator.doStop` and `saveState()` on the executor), so one writer can move the other's half-written file into place. A unique temp file avoids this. It's the same call `FeatureConfigInstaller` already uses (lines 329 and 413): ```suggestion File tmpFile = File.createTempFile(file.getName(), ".tmp", file.getAbsoluteFile().getParentFile()); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
