[bugfix] Ensure that the HTTP response OutputStream is only closed if an exception does not occur during serialization

Author: adamretter

exist-core/src/main/java/org/exist/http/RESTServer.java

@@ -111,6 +111,8 @@
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
+
+import javax.annotation.Nullable;
 import javax.xml.XMLConstants;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.stream.XMLStreamException;
@@ -1827,14 +1829,22 @@ private void writeResourceAs(final DocumentImpl resource, final DBBroker broker,
                     outputProperties.setProperty("omit-xml-declaration", "no");
                 }
 
-                final OutputStreamWriter writer = new OutputStreamWriter(response.getOutputStream(), encoding);
-                sax.setOutput(writer, outputProperties);
-                serializer.setSAXHandlers(sax, sax);
+                // NOTE(AR) we only close the OutputStreamWriter if serialization succeeds, otherwise we raise a BadRequestException below which needs the OutputStream to remain open so that it can report the issue via the HTTP response to the client
+                @Nullable Writer writerToClose = null;
+                try {
+                    final Writer writer = new OutputStreamWriter(response.getOutputStream(), encoding);
+                    sax.setOutput(writer, outputProperties);
+                    serializer.setSAXHandlers(sax, sax);
 
-                serializer.toSAX(resource);
+                    serializer.toSAX(resource);
 
-                writer.flush();
-                writer.close(); // DO NOT use in try-write-resources, otherwise ther response stream is always closed, and we can't report the errors
+                    writer.flush();
+                    writerToClose = writer;
+                } finally {
+                    if (writerToClose != null) {
+                        writerToClose.close();
+                    }
+                }
             } catch (final SAXException saxe) {
                 LOG.warn(saxe);
                 throw new BadRequestException("Error while serializing XML: " + saxe.getMessage());
@@ -1870,28 +1880,36 @@ private void writeXPathExceptionHtml(final HttpServletResponse response,
 
         response.setContentType(MimeType.HTML_TYPE.getName() + "; charset=" + encoding);
 
-        final OutputStreamWriter writer = new OutputStreamWriter(response.getOutputStream(), encoding);
-        writer.write(QUERY_ERROR_HEAD);
-        writer.write("<p class=\"path\"><span class=\"high\">Path</span>: ");
-        writer.write("<a href=\"");
-        writer.write(path);
-        writer.write("\">");
-        writer.write(path);
-        writer.write("</a>");
-
-        writer.write("<p class=\"errmsg\">");
-        final String message = e.getMessage() == null ? e.toString() : e.getMessage();
-        writer.write(XMLUtil.encodeAttrMarkup(message));
-        writer.write("");
-        if (query != null) {
-            writer.write("<span class=\"high\">Query</span>:<pre>");
-            writer.write(XMLUtil.encodeAttrMarkup(query));
-            writer.write("</pre>");
-        }
-        writer.write("</body></html>");
+        // NOTE(AR) we only close the OutputStreamWriter if serialization succeeds, otherwise we raise a BadRequestException below which needs the OutputStream to remain open so that it can report the issue via the HTTP response to the client
+        @Nullable Writer writerToClose = null;
+        try {
+            final Writer writer = new OutputStreamWriter(response.getOutputStream(), encoding);
+            writer.write(QUERY_ERROR_HEAD);
+            writer.write("<p class=\"path\"><span class=\"high\">Path</span>: ");
+            writer.write("<a href=\"");
+            writer.write(path);
+            writer.write("\">");
+            writer.write(path);
+            writer.write("</a>");
 
-        writer.flush();
-        writer.close();
+            writer.write("<p class=\"errmsg\">");
+            final String message = e.getMessage() == null ? e.toString() : e.getMessage();
+            writer.write(XMLUtil.encodeAttrMarkup(message));
+            writer.write("");
+            if (query != null) {
+                writer.write("<span class=\"high\">Query</span>:<pre>");
+                writer.write(XMLUtil.encodeAttrMarkup(query));
+                writer.write("</pre>");
+            }
+            writer.write("</body></html>");
+
+            writer.flush();
+            writerToClose = writer;
+        } finally {
+            if (writerToClose != null) {
+                writerToClose.close();
+            }
+        }
     }
 
     /**
@@ -1913,8 +1931,10 @@ private void writeXPathException(final HttpServletResponse response,
 
         response.setContentType(MimeType.XML_TYPE.getName() + "; charset=" + encoding);
 
-        try(final OutputStreamWriter writer =
-                new OutputStreamWriter(response.getOutputStream(), encoding)) {
+        // NOTE(AR) we only close the OutputStreamWriter if serialization succeeds, otherwise we raise a BadRequestException below which needs the OutputStream to remain open so that it can report the issue via the HTTP response to the client
+        @Nullable Writer writerToClose = null;
+        try {
+            final Writer writer = new OutputStreamWriter(response.getOutputStream(), encoding);
 
             writer.write("<?xml version=\"1.0\" ?>");
             writer.write("<exception><path>");
@@ -1930,6 +1950,13 @@ private void writeXPathException(final HttpServletResponse response,
                 writer.write("</query>");
             }
             writer.write("</exception>");
+
+            writer.flush();
+            writerToClose = writer;
+        } finally {
+            if (writerToClose != null) {
+                writerToClose.close();
+            }
         }
     }
 
@@ -1947,17 +1974,23 @@ private void writeXUpdateResult(final HttpServletResponse response,
 
         response.setContentType(MimeType.XML_TYPE.getName() + "; charset=" + encoding);
 
-        final OutputStreamWriter writer =
-                new OutputStreamWriter(response.getOutputStream(), encoding);
-
-        writer.write("<?xml version=\"1.0\" ?>");
-        writer.write("<exist:modifications xmlns:exist=\""
+        // NOTE(AR) we only close the OutputStreamWriter if serialization succeeds, otherwise we raise a BadRequestException below which needs the OutputStream to remain open so that it can report the issue via the HTTP response to the client
+        @Nullable Writer writerToClose = null;
+        try {
+            final Writer writer = new OutputStreamWriter(response.getOutputStream(), encoding);
+            writer.write("<?xml version=\"1.0\" ?>");
+            writer.write("<exist:modifications xmlns:exist=\""
                 + Namespaces.EXIST_NS + "\" count=\"" + updateCount + "\">");
-        writer.write(updateCount + " modifications processed.");
-        writer.write("</exist:modifications>");
+            writer.write(updateCount + " modifications processed.");
+            writer.write("</exist:modifications>");
 
-        writer.flush();
-        writer.close();
+            writer.flush();
+            writerToClose = writer;
+        } finally {
+            if (writerToClose != null) {
+                writerToClose.close();
+            }
+        }
     }
 
     /**
@@ -1980,12 +2013,12 @@ protected void writeCollection(final HttpServletResponse response,
 
         setCreatedAndLastModifiedHeaders(response, collection.getCreated(), collection.getCreated());
 
-        final OutputStreamWriter writer =
-                new OutputStreamWriter(response.getOutputStream(), encoding);
-
         SAXSerializer serializer = null;
 
+        // NOTE(AR) we only close the OutputStreamWriter if serialization succeeds, otherwise we raise a BadRequestException below which needs the OutputStream to remain open so that it can report the issue via the HTTP response to the client
+        @Nullable Writer writerToClose = null;
         try {
+            final Writer writer = new OutputStreamWriter(response.getOutputStream(), encoding);
             serializer = (SAXSerializer) SerializerPool.getInstance().borrowObject(SAXSerializer.class);
 
             serializer.setOutput(writer, defaultProperties);
@@ -2085,7 +2118,7 @@ protected void writeCollection(final HttpServletResponse response,
             serializer.endDocument();
 
             writer.flush();
-            writer.close();
+            writerToClose = writer;
 
         } catch (final SAXException e) {
             // should never happen
@@ -2094,6 +2127,9 @@ protected void writeCollection(final HttpServletResponse response,
             if (serializer != null) {
                 SerializerPool.getInstance().returnObject(serializer);
             }
+            if (writerToClose != null) {
+                writerToClose.close();
+            }
         }
     }
 
@@ -2165,14 +2201,23 @@ private void writeResultXML(final HttpServletResponse response,
             if (wrap) {
                 outputProperties.setProperty("method", "xml");
             }
-            final Writer writer = new OutputStreamWriter(response.getOutputStream(), encoding);
-            final XQuerySerializer serializer = new XQuerySerializer(broker, outputProperties, writer);
 
-            //Marshaller.marshall(broker, results, start, howmany, serializer.getContentHandler());
-            serializer.serialize(results, start, howmany, wrap, typed, compilationTime, executionTime);
+            // NOTE(AR) we only close the OutputStreamWriter if serialization succeeds, otherwise we raise a BadRequestException below which needs the OutputStream to remain open so that it can report the issue via the HTTP response to the client
+            @Nullable Writer writerToClose = null;
+            try {
+                final Writer writer = new OutputStreamWriter(response.getOutputStream(), encoding);
+                final XQuerySerializer serializer = new XQuerySerializer(broker, outputProperties, writer);
 
-            writer.flush();
-            writer.close();
+                //Marshaller.marshall(broker, results, start, howmany, serializer.getContentHandler());
+                serializer.serialize(results, start, howmany, wrap, typed, compilationTime, executionTime);
+
+                writer.flush();
+                writerToClose = writer;
+            } finally {
+                if (writerToClose != null) {
+                    writerToClose.close();
+                }
+            }
 
         } catch (final SAXException e) {
             LOG.warn(e);
@@ -2208,7 +2253,9 @@ private void writeResultJSON(final HttpServletResponse response,
         outputProperties.setProperty(Serializer.GENERATE_DOC_EVENTS, "false");
         try {
             serializer.setProperties(outputProperties);
-            try (final Writer writer = new OutputStreamWriter(response.getOutputStream(), outputProperties.getProperty(OutputKeys.ENCODING))) {
+            @Nullable Writer writerToClose = null;
+            try {
+                final Writer writer = new OutputStreamWriter(response.getOutputStream(), outputProperties.getProperty(OutputKeys.ENCODING));
                 final JSONObject root = new JSONObject();
                 root.addObject(new JSONSimpleProperty("start", Integer.toString(start), true));
                 root.addObject(new JSONSimpleProperty("count", Integer.toString(howmany), true));
@@ -2252,6 +2299,12 @@ private void writeResultJSON(final HttpServletResponse response,
                 root.serialize(writer, true);
 
                 writer.flush();
+                writerToClose = writer;
+
+            } finally {
+                if (writerToClose != null) {
+                    writerToClose.close();
+                }
             }
         } catch (final IOException | XPathException | SAXException e) {
             throw new BadRequestException("Error while serializing xml: " + e.toString(), e);

exist-core/src/test/java/org/exist/storage/XIncludeSerializerTest.java

@@ -289,12 +289,13 @@ public void fallback2() throws IOException {
         connect.setRequestMethod("GET");
         connect.connect();
 
-        final BufferedReader reader = new BufferedReader(new InputStreamReader(connect.getInputStream(), "UTF-8"));
-        String line;
         final StringBuilder out = new StringBuilder();
-        while ((line = reader.readLine()) != null) {
-            out.append(line);
-            out.append("\r\n");
+        try (final BufferedReader reader = new BufferedReader(new InputStreamReader(connect.getInputStream(), "UTF-8"))) {
+            String line;
+            while ((line = reader.readLine()) != null) {
+                out.append(line);
+                out.append("\r\n");
+            }
         }
         final String responseXML = out.toString();
     }