From b26ed95333ef72d7460655feee1471dc6370b3da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Mon, 22 Nov 2021 10:26:00 +0100 Subject: [PATCH] Autocloseable streams in XML DB (#1868) Introduce autocloseable streams for file handling in xml database module. --- gradle/changelog/windows_stream_handling.yaml | 4 + .../java/sonia/scm/BaseDirectoryTest.java | 1 - .../scm/repository/xml/PathDatabase.java | 47 ++-- .../sonia/scm/store/FileStoreExporter.java | 10 +- .../store/JAXBConfigurationEntryStore.java | 22 +- .../main/java/sonia/scm/xml/XmlStreams.java | 235 +++++++++++++++++- ...BetweenConfigAndConfigEntryUpdateStep.java | 19 +- 7 files changed, 266 insertions(+), 72 deletions(-) create mode 100644 gradle/changelog/windows_stream_handling.yaml diff --git a/gradle/changelog/windows_stream_handling.yaml b/gradle/changelog/windows_stream_handling.yaml new file mode 100644 index 0000000000..8941cfdfba --- /dev/null +++ b/gradle/changelog/windows_stream_handling.yaml @@ -0,0 +1,4 @@ +- type: fixed + description: Closing of file streams ([#1857](https://github.com/scm-manager/scm-manager/pull/1857) and [#1868](https://github.com/scm-manager/scm-manager/pull/1868)) +- type: fixed + description: Exit of retry loop for deletion of files ([#1857](https://github.com/scm-manager/scm-manager/pull/1857) and [#1868](https://github.com/scm-manager/scm-manager/pull/1868)) diff --git a/scm-core/src/test/java/sonia/scm/BaseDirectoryTest.java b/scm-core/src/test/java/sonia/scm/BaseDirectoryTest.java index 6d8e63a9ed..d33dae1306 100644 --- a/scm-core/src/test/java/sonia/scm/BaseDirectoryTest.java +++ b/scm-core/src/test/java/sonia/scm/BaseDirectoryTest.java @@ -28,7 +28,6 @@ import com.google.common.collect.ImmutableMap; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; -import java.io.File; import java.nio.file.Paths; import java.util.Map; import java.util.Properties; diff --git a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/PathDatabase.java b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/PathDatabase.java index a807ed35f8..739d0ef4b9 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/PathDatabase.java +++ b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/PathDatabase.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.repository.xml; import org.slf4j.Logger; @@ -29,16 +29,15 @@ import org.slf4j.LoggerFactory; import sonia.scm.ContextEntry; import sonia.scm.repository.InternalRepositoryException; import sonia.scm.store.CopyOnWrite; -import sonia.scm.xml.IndentXMLStreamWriter; import sonia.scm.xml.XmlStreams; +import sonia.scm.xml.XmlStreams.AutoCloseableXMLReader; +import sonia.scm.xml.XmlStreams.AutoCloseableXMLWriter; +import javax.xml.stream.XMLStreamConstants; import javax.xml.stream.XMLStreamException; import javax.xml.stream.XMLStreamReader; import javax.xml.stream.XMLStreamWriter; import java.io.IOException; -import java.io.Reader; -import java.io.Writer; -import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; @@ -70,24 +69,22 @@ class PathDatabase { CopyOnWrite.withTemporaryFile( temp -> { - try (Writer ioWriter = Files.newBufferedWriter(temp, StandardCharsets.UTF_8)) { - try (IndentXMLStreamWriter writer = XmlStreams.createWriter(ioWriter)) { - writer.writeStartDocument(ENCODING, VERSION); + try (AutoCloseableXMLWriter writer = XmlStreams.createWriter(temp)) { + writer.writeStartDocument(ENCODING, VERSION); - writeRepositoriesStart(writer, creationTime, lastModified); - for (Map.Entry e : pathDatabase.entrySet()) { - writeRepository(writer, e.getKey(), e.getValue()); - } - writer.writeEndElement(); - - writer.writeEndDocument(); - } catch (XMLStreamException ex) { - throw new InternalRepositoryException( - ContextEntry.ContextBuilder.entity(Path.class, storePath.toString()).build(), - "failed to write repository path database", - ex - ); + writeRepositoriesStart(writer, creationTime, lastModified); + for (Map.Entry e : pathDatabase.entrySet()) { + writeRepository(writer, e.getKey(), e.getValue()); } + writer.writeEndElement(); + + writer.writeEndDocument(); + } catch (XMLStreamException | IOException ex) { + throw new InternalRepositoryException( + ContextEntry.ContextBuilder.entity(Path.class, storePath.toString()).build(), + "failed to write repository path database", + ex + ); } }, storePath @@ -125,14 +122,12 @@ class PathDatabase { void read(OnRepositories onRepositories, OnRepository onRepository) { LOG.trace("read repository path database from {}", storePath); - XMLStreamReader reader = null; - try (Reader inputReader = Files.newBufferedReader(storePath, StandardCharsets.UTF_8)) { - reader = XmlStreams.createReader(inputReader); + try (AutoCloseableXMLReader reader = XmlStreams.createReader(storePath)) { while (reader.hasNext()) { int eventType = reader.next(); - if (eventType == XMLStreamReader.START_ELEMENT) { + if (eventType == XMLStreamConstants.START_ELEMENT) { String element = reader.getLocalName(); if (ELEMENT_REPOSITORIES.equals(element)) { readRepositories(reader, onRepositories); @@ -147,8 +142,6 @@ class PathDatabase { "failed to read repository path database", ex ); - } finally { - XmlStreams.close(reader); } } diff --git a/scm-dao-xml/src/main/java/sonia/scm/store/FileStoreExporter.java b/scm-dao-xml/src/main/java/sonia/scm/store/FileStoreExporter.java index 654101f131..853a903aa6 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/store/FileStoreExporter.java +++ b/scm-dao-xml/src/main/java/sonia/scm/store/FileStoreExporter.java @@ -30,13 +30,11 @@ import sonia.scm.repository.Repository; import sonia.scm.repository.RepositoryLocationResolver; import sonia.scm.repository.api.ExportFailedException; import sonia.scm.xml.XmlStreams; +import sonia.scm.xml.XmlStreams.AutoCloseableXMLReader; import javax.inject.Inject; import javax.xml.stream.XMLStreamException; -import javax.xml.stream.XMLStreamReader; import java.io.IOException; -import java.io.Reader; -import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; @@ -136,9 +134,7 @@ public class FileStoreExporter implements StoreExporter { } private StoreType determineConfigType(Path storePath) { - XMLStreamReader reader = null; - try (Reader inputReader = Files.newBufferedReader(storePath, StandardCharsets.UTF_8)) { - reader = XmlStreams.createReader(inputReader); + try (AutoCloseableXMLReader reader = XmlStreams.createReader(storePath)) { reader.nextTag(); if ( "configuration".equals(reader.getLocalName()) @@ -150,8 +146,6 @@ public class FileStoreExporter implements StoreExporter { } } catch (XMLStreamException | IOException e) { throw new ExportFailedException(noContext(), "Failed to read store file " + storePath, e); - } finally { - XmlStreams.close(reader); } } } diff --git a/scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java b/scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java index 33f6792447..9ae042a3b8 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java +++ b/scm-dao-xml/src/main/java/sonia/scm/store/JAXBConfigurationEntryStore.java @@ -29,18 +29,14 @@ import com.google.common.collect.Maps; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sonia.scm.security.KeyGenerator; -import sonia.scm.xml.IndentXMLStreamWriter; import sonia.scm.xml.XmlStreams; +import sonia.scm.xml.XmlStreams.AutoCloseableXMLReader; +import sonia.scm.xml.XmlStreams.AutoCloseableXMLWriter; import javax.xml.bind.JAXBElement; import javax.xml.bind.Marshaller; import javax.xml.namespace.QName; -import javax.xml.stream.XMLStreamReader; import java.io.File; -import java.io.Reader; -import java.io.Writer; -import java.nio.charset.StandardCharsets; -import java.nio.file.Files; import java.util.Collection; import java.util.Collections; import java.util.Map; @@ -141,10 +137,7 @@ public class JAXBConfigurationEntryStore implements ConfigurationEntryStore { - XMLStreamReader reader = null; - - try (Reader inputReader = Files.newBufferedReader(file.toPath(), StandardCharsets.UTF_8)) { - reader = XmlStreams.createReader(inputReader); + try (AutoCloseableXMLReader reader = XmlStreams.createReader(file)) { // configuration reader.nextTag(); @@ -181,8 +174,6 @@ public class JAXBConfigurationEntryStore implements ConfigurationEntryStore implements ConfigurationEntryStore { - m.setProperty(Marshaller.JAXB_FRAGMENT, Boolean.TRUE); + m.setProperty(Marshaller.JAXB_FRAGMENT, Boolean.TRUE); CopyOnWrite.withTemporaryFile( temp -> { - try (Writer ioWriter = Files.newBufferedWriter(temp, StandardCharsets.UTF_8); - IndentXMLStreamWriter writer = XmlStreams.createWriter(ioWriter)) { + try (AutoCloseableXMLWriter writer = XmlStreams.createWriter(temp)) { writer.writeStartDocument(); // configuration start @@ -217,7 +207,7 @@ public class JAXBConfigurationEntryStore implements ConfigurationEntryStore je = new JAXBElement<>(QName.valueOf(TAG_VALUE), type, - e.getValue()); + e.getValue()); m.marshal(je, writer); diff --git a/scm-dao-xml/src/main/java/sonia/scm/xml/XmlStreams.java b/scm-dao-xml/src/main/java/sonia/scm/xml/XmlStreams.java index c15b770952..4b5b649d29 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/xml/XmlStreams.java +++ b/scm-dao-xml/src/main/java/sonia/scm/xml/XmlStreams.java @@ -21,19 +21,27 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.xml; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import javax.xml.namespace.NamespaceContext; import javax.xml.stream.XMLInputFactory; import javax.xml.stream.XMLOutputFactory; import javax.xml.stream.XMLStreamException; import javax.xml.stream.XMLStreamReader; import javax.xml.stream.XMLStreamWriter; +import javax.xml.stream.util.StreamReaderDelegate; +import java.io.Closeable; +import java.io.File; +import java.io.IOException; import java.io.Reader; import java.io.Writer; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; public final class XmlStreams { @@ -62,13 +70,226 @@ public final class XmlStreams { } } - public static XMLStreamReader createReader(Reader reader) throws XMLStreamException { - XMLInputFactory factory = XMLInputFactory.newInstance(); - factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, Boolean.FALSE); - return factory.createXMLStreamReader(reader); + public static AutoCloseableXMLReader createReader(Path path) throws IOException, XMLStreamException { + return createReader(Files.newBufferedReader(path, StandardCharsets.UTF_8)); } - public static IndentXMLStreamWriter createWriter(Writer writer) throws XMLStreamException { - return new IndentXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(writer)); + public static AutoCloseableXMLReader createReader(File file) throws IOException, XMLStreamException { + return createReader(file.toPath()); + } + + private static AutoCloseableXMLReader createReader(Reader reader) throws XMLStreamException { + XMLInputFactory factory = XMLInputFactory.newInstance(); + factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, Boolean.FALSE); + XMLStreamReader xmlStreamReader = factory.createXMLStreamReader(reader); + return new AutoCloseableXMLReader(xmlStreamReader, reader); + } + + public static AutoCloseableXMLWriter createWriter(Path path) throws IOException, XMLStreamException { + return createWriter(Files.newBufferedWriter(path, StandardCharsets.UTF_8)); + } + + public static AutoCloseableXMLWriter createWriter(File file) throws IOException, XMLStreamException { + return createWriter(file.toPath()); + } + + private static AutoCloseableXMLWriter createWriter(Writer writer) throws XMLStreamException { + IndentXMLStreamWriter indentXMLStreamWriter = new IndentXMLStreamWriter(XMLOutputFactory.newFactory().createXMLStreamWriter(writer)); + return new AutoCloseableXMLWriter(indentXMLStreamWriter, writer); + } + + public static final class AutoCloseableXMLReader extends StreamReaderDelegate implements AutoCloseable { + + private final Closeable closeable; + + public AutoCloseableXMLReader(XMLStreamReader delegate, Closeable closeable) { + super(delegate); + this.closeable = closeable; + } + + @Override + public void close() throws XMLStreamException { + super.close(); + try { + closeable.close(); + } catch (IOException e) { + throw new XMLStreamException("failed to close nested reader", e); + } + } + } + + public static final class AutoCloseableXMLWriter implements XMLStreamWriter, AutoCloseable { + private final XMLStreamWriter delegate; + private final Closeable closeable; + + public AutoCloseableXMLWriter(XMLStreamWriter delegate, Closeable closeable) { + this.delegate = delegate; + this.closeable = closeable; + } + + @Override + public void writeStartElement(String localName) throws XMLStreamException { + delegate.writeStartElement(localName); + } + + @Override + public void writeStartElement(String namespaceURI, String localName) throws XMLStreamException { + delegate.writeStartElement(namespaceURI, localName); + } + + @Override + public void writeStartElement(String prefix, String localName, String namespaceURI) throws XMLStreamException { + delegate.writeStartElement(prefix, localName, namespaceURI); + } + + @Override + public void writeEmptyElement(String namespaceURI, String localName) throws XMLStreamException { + delegate.writeEmptyElement(namespaceURI, localName); + } + + @Override + public void writeEmptyElement(String prefix, String localName, String namespaceURI) throws XMLStreamException { + delegate.writeEmptyElement(prefix, localName, namespaceURI); + } + + @Override + public void writeEmptyElement(String localName) throws XMLStreamException { + delegate.writeEmptyElement(localName); + } + + @Override + public void writeEndElement() throws XMLStreamException { + delegate.writeEndElement(); + } + + @Override + public void writeEndDocument() throws XMLStreamException { + delegate.writeEndDocument(); + } + + @Override + public void close() throws XMLStreamException { + delegate.close(); + try { + closeable.close(); + } catch (IOException e) { + throw new XMLStreamException("failed to close nested writer", e); + } + } + + @Override + public void flush() throws XMLStreamException { + delegate.flush(); + } + + @Override + public void writeAttribute(String localName, String value) throws XMLStreamException { + delegate.writeAttribute(localName, value); + } + + @Override + public void writeAttribute(String prefix, String namespaceURI, String localName, String value) throws XMLStreamException { + delegate.writeAttribute(prefix, namespaceURI, localName, value); + } + + @Override + public void writeAttribute(String namespaceURI, String localName, String value) throws XMLStreamException { + delegate.writeAttribute(namespaceURI, localName, value); + } + + @Override + public void writeNamespace(String prefix, String namespaceURI) throws XMLStreamException { + delegate.writeNamespace(prefix, namespaceURI); + } + + @Override + public void writeDefaultNamespace(String namespaceURI) throws XMLStreamException { + delegate.writeDefaultNamespace(namespaceURI); + } + + @Override + public void writeComment(String data) throws XMLStreamException { + delegate.writeComment(data); + } + + @Override + public void writeProcessingInstruction(String target) throws XMLStreamException { + delegate.writeProcessingInstruction(target); + } + + @Override + public void writeProcessingInstruction(String target, String data) throws XMLStreamException { + delegate.writeProcessingInstruction(target, data); + } + + @Override + public void writeCData(String data) throws XMLStreamException { + delegate.writeCData(data); + } + + @Override + public void writeDTD(String dtd) throws XMLStreamException { + delegate.writeDTD(dtd); + } + + @Override + public void writeEntityRef(String name) throws XMLStreamException { + delegate.writeEntityRef(name); + } + + @Override + public void writeStartDocument() throws XMLStreamException { + delegate.writeStartDocument(); + } + + @Override + public void writeStartDocument(String version) throws XMLStreamException { + delegate.writeStartDocument(version); + } + + @Override + public void writeStartDocument(String encoding, String version) throws XMLStreamException { + delegate.writeStartDocument(encoding, version); + } + + @Override + public void writeCharacters(String text) throws XMLStreamException { + delegate.writeCharacters(text); + } + + @Override + public void writeCharacters(char[] text, int start, int len) throws XMLStreamException { + delegate.writeCharacters(text, start, len); + } + + @Override + public String getPrefix(String uri) throws XMLStreamException { + return delegate.getPrefix(uri); + } + + @Override + public void setPrefix(String prefix, String uri) throws XMLStreamException { + delegate.setPrefix(prefix, uri); + } + + @Override + public void setDefaultNamespace(String uri) throws XMLStreamException { + delegate.setDefaultNamespace(uri); + } + + @Override + public void setNamespaceContext(NamespaceContext context) throws XMLStreamException { + delegate.setNamespaceContext(context); + } + + @Override + public NamespaceContext getNamespaceContext() { + return delegate.getNamespaceContext(); + } + + @Override + public Object getProperty(String name) throws IllegalArgumentException { + return delegate.getProperty(name); + } } } diff --git a/scm-webapp/src/main/java/sonia/scm/update/store/DifferentiateBetweenConfigAndConfigEntryUpdateStep.java b/scm-webapp/src/main/java/sonia/scm/update/store/DifferentiateBetweenConfigAndConfigEntryUpdateStep.java index 942aeb6d35..9ffcc652b1 100644 --- a/scm-webapp/src/main/java/sonia/scm/update/store/DifferentiateBetweenConfigAndConfigEntryUpdateStep.java +++ b/scm-webapp/src/main/java/sonia/scm/update/store/DifferentiateBetweenConfigAndConfigEntryUpdateStep.java @@ -32,12 +32,12 @@ import sonia.scm.migration.UpdateException; import sonia.scm.store.CopyOnWrite; import sonia.scm.version.Version; import sonia.scm.xml.XmlStreams; +import sonia.scm.xml.XmlStreams.AutoCloseableXMLReader; import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.stream.XMLStreamException; -import javax.xml.stream.XMLStreamReader; import javax.xml.transform.TransformerException; import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; @@ -45,8 +45,6 @@ import javax.xml.transform.stream.StreamResult; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.io.Reader; -import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.stream.Stream; @@ -75,9 +73,7 @@ abstract class DifferentiateBetweenConfigAndConfigEntryUpdateStep { private boolean isConfigEntryFile(Path potentialFile) { LOG.trace("Testing whether file is config entry file without mark: {}", potentialFile); - XMLStreamReader reader = null; - try (Reader inputReader = Files.newBufferedReader(potentialFile, StandardCharsets.UTF_8)) { - reader = XmlStreams.createReader(inputReader); + try (AutoCloseableXMLReader reader = XmlStreams.createReader(potentialFile)) { reader.nextTag(); if ("configuration".equals(reader.getLocalName())) { reader.nextTag(); @@ -87,8 +83,6 @@ abstract class DifferentiateBetweenConfigAndConfigEntryUpdateStep { } } catch (XMLStreamException | IOException e) { throw new UpdateException("Error reading file " + potentialFile, e); - } finally { - XmlStreams.close(reader); } return false; } @@ -106,12 +100,11 @@ abstract class DifferentiateBetweenConfigAndConfigEntryUpdateStep { } private void writeXmlDocument(Document configEntryDocument, Path temporaryFile) throws TransformerException { + TransformerFactory factory = TransformerFactory.newInstance(); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); + DOMSource domSource = new DOMSource(configEntryDocument); try (OutputStream os = Files.newOutputStream(temporaryFile)) { - final TransformerFactory factory = TransformerFactory.newInstance(); - factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); - factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); - final DOMSource domSource = new DOMSource(configEntryDocument); - factory.newTransformer().transform(domSource, new StreamResult(os)); } catch (IOException e) { throw new UpdateException("Could not write modified config entry file", e);