You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by en...@apache.org on 2021/06/14 18:41:07 UTC
[sling-org-apache-sling-servlets-resolver] branch master updated:
SLING-10478 do not attempt to close an already closed writer (#15)
This is an automated email from the ASF dual-hosted git repository.
enorman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-servlets-resolver.git
The following commit(s) were added to refs/heads/master by this push:
new 7c0a1fe SLING-10478 do not attempt to close an already closed writer (#15)
7c0a1fe is described below
commit 7c0a1fe63de1a41559db35b4153635993038550b
Author: Eric Norman <er...@gmail.com>
AuthorDate: Mon Jun 14 11:40:59 2021 -0700
SLING-10478 do not attempt to close an already closed writer (#15)
SlingServletResolver#handleError should not attempt to flush/close an
already closed response writer. Added integration tests to verify the fix.
---
.../internal/HandleErrorResponseWriter.java | 46 ++++++
.../HandleErrorSlingHttpServletResponse.java | 54 ++++++
.../resolver/internal/SlingServletResolver.java | 20 ++-
.../sling/servlets/resolver/it/SLING10478IT.java | 182 +++++++++++++++++++++
.../resolver/it/ServletResolverTestSupport.java | 10 +-
5 files changed, 305 insertions(+), 7 deletions(-)
diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/HandleErrorResponseWriter.java b/src/main/java/org/apache/sling/servlets/resolver/internal/HandleErrorResponseWriter.java
new file mode 100644
index 0000000..8a17b5f
--- /dev/null
+++ b/src/main/java/org/apache/sling/servlets/resolver/internal/HandleErrorResponseWriter.java
@@ -0,0 +1,46 @@
+/*
+ * 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.sling.servlets.resolver.internal;
+
+import java.io.PrintWriter;
+import java.io.Writer;
+
+/**
+ * Wrap the original PrintWriter so we can monitor if it
+ * has been closed
+ */
+class HandleErrorResponseWriter extends PrintWriter {
+
+ private boolean open = true;
+
+ HandleErrorResponseWriter(Writer out) {
+ super(out);
+ }
+
+ @Override
+ public void close() {
+ this.open = false;
+ super.close();
+ }
+
+ public boolean isOpen() {
+ return open;
+ }
+
+}
diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/HandleErrorSlingHttpServletResponse.java b/src/main/java/org/apache/sling/servlets/resolver/internal/HandleErrorSlingHttpServletResponse.java
new file mode 100644
index 0000000..c4c4b8f
--- /dev/null
+++ b/src/main/java/org/apache/sling/servlets/resolver/internal/HandleErrorSlingHttpServletResponse.java
@@ -0,0 +1,54 @@
+/*
+ * 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.sling.servlets.resolver.internal;
+
+import java.io.IOException;
+import java.io.PrintWriter;
+
+import org.apache.sling.api.SlingHttpServletResponse;
+import org.apache.sling.api.wrappers.SlingHttpServletResponseWrapper;
+
+/**
+ * wrap the original response so we can monitor if the writer
+ * has been closed
+ */
+final class HandleErrorSlingHttpServletResponse extends SlingHttpServletResponseWrapper {
+ private HandleErrorResponseWriter writer = null;
+
+ HandleErrorSlingHttpServletResponse(SlingHttpServletResponse response) {
+ super(response);
+ }
+
+ @Override
+ public PrintWriter getWriter() throws IOException {
+ if (this.writer == null) {
+ this.writer = new HandleErrorResponseWriter(getResponse().getWriter());
+ }
+ return this.writer;
+ }
+
+ /**
+ * Returns whether the response writer is open
+ * @return true of open, false otherwise
+ */
+ public boolean isOpen() {
+ return this.writer.isOpen();
+ }
+
+}
diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java b/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java
index d72c72a..7318608 100644
--- a/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java
+++ b/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java
@@ -628,7 +628,7 @@ public class SlingServletResolver
return fallbackErrorServlet;
}
- private void handleError(final Servlet errorHandler, final HttpServletRequest request, final HttpServletResponse response)
+ private void handleError(final Servlet errorHandler, final SlingHttpServletRequest request, final SlingHttpServletResponse response)
throws IOException {
request.setAttribute(SlingConstants.ERROR_REQUEST_URI, request.getRequestURI());
@@ -643,11 +643,19 @@ public class SlingServletResolver
// forward all exceptions if it fails.
// Before SLING-4143 we only forwarded IOExceptions.
try {
- errorHandler.service(request, response);
- // commit the response
- response.flushBuffer();
- // close the response (SLING-2724)
- response.getWriter().close();
+ // SLING-10478 - wrap the response to track if the writer is still open
+ // after the errorHandler has serviced the request
+ HandleErrorSlingHttpServletResponse wrappedResponse = new HandleErrorSlingHttpServletResponse(response);
+
+ errorHandler.service(request, wrappedResponse);
+
+ // SLING-10478 - if the response writer has not already been closed, then flush and close it
+ if (wrappedResponse.isOpen()) {
+ // commit the response
+ wrappedResponse.flushBuffer();
+ // close the response (SLING-2724)
+ wrappedResponse.getWriter().close();
+ }
} catch (final Throwable t) {
LOGGER.error("Calling the error handler resulted in an error", t);
LOGGER.error("Original error " + request.getAttribute(SlingConstants.ERROR_EXCEPTION_TYPE),
diff --git a/src/test/java/org/apache/sling/servlets/resolver/it/SLING10478IT.java b/src/test/java/org/apache/sling/servlets/resolver/it/SLING10478IT.java
new file mode 100644
index 0000000..1453697
--- /dev/null
+++ b/src/test/java/org/apache/sling/servlets/resolver/it/SLING10478IT.java
@@ -0,0 +1,182 @@
+/*
+ * 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.sling.servlets.resolver.it;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.Writer;
+import java.lang.reflect.InvocationTargetException;
+
+import javax.json.Json;
+import javax.json.stream.JsonGenerator;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.apache.sling.servlethelpers.MockSlingHttpServletResponse;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.ops4j.pax.exam.junit.PaxExam;
+import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy;
+import org.ops4j.pax.exam.spi.reactors.PerClass;
+
+/**
+ * Tests for SLING-10478 - SlingServletResolver#handleError should not attempt to flush/close
+ * an already closed response writer
+ */
+@RunWith(PaxExam.class)
+@ExamReactorStrategy(PerClass.class)
+public class SLING10478IT extends ServletResolverTestSupport {
+ public static final String P_PREFIX = "sling.servlet.prefix";
+
+ @Before
+ public void setupTestServlets() throws Exception {
+ new TestServlet("CloseWriterErrorHandler") {
+ private static final long serialVersionUID = 5467958588168607027L;
+
+ @Override
+ public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+ resp.setStatus(HttpServletResponse.SC_NOT_FOUND);
+ resp.setCharacterEncoding("UTF-8");
+ resp.setContentType("application/json");
+ // closing the JsonGenerator also closes the response writer
+ try (JsonGenerator generator = Json.createGenerator(resp.getWriter())) {
+ generator.writeStartObject();
+ generator.write("hello", "error");
+ generator.writeEnd();
+ }
+ }
+
+ }
+ .with(P_RESOURCE_TYPES, "sling/servlet/errorhandler")
+ .with(P_EXTENSIONS, "close")
+ .with(P_METHODS, "default")
+ .with(P_PREFIX, -1)
+ .register(bundleContext);
+
+ new TestServlet("NoCloseWriterErrorHandler") {
+ private static final long serialVersionUID = 5467958588168607027L;
+
+ @Override
+ public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException {
+ resp.setStatus(HttpServletResponse.SC_NOT_FOUND);
+ resp.setCharacterEncoding("UTF-8");
+ resp.setContentType("text/plain");
+
+ resp.getWriter().print("hello error");
+
+ // response writer remains unclosed
+ }
+
+ }
+ .with(P_RESOURCE_TYPES, "sling/servlet/errorhandler")
+ .with(P_EXTENSIONS, "noclose")
+ .with(P_METHODS, "default")
+ .with(P_PREFIX, -1)
+ .register(bundleContext);
+ }
+
+ @Override
+ protected MockSlingHttpServletResponse createMockSlingHttpServletResponse() {
+ return new HandleErrorMockSlingHttpServletResponse();
+ }
+
+ /**
+ * Test an error handling servlet that closes the response writer
+ */
+ @Test
+ public void testCloseWriterErrorHandlerServlet() throws Exception {
+ checkErrorHandlerServlet("/not_real_path.close");
+ }
+
+ /**
+ * Test an error handling servlet that does not close the response writer
+ */
+ @Test
+ public void testNoCloseWriterErrorHandlerServlet() throws Exception {
+ checkErrorHandlerServlet("/not_real_path.noclose");
+ }
+
+ void checkErrorHandlerServlet(String path) throws Exception, InvocationTargetException {
+ try {
+ final String output = executeRequest(M_GET, path, HttpServletResponse.SC_NOT_FOUND).getOutputAsString();
+ assertNotNull(output);
+ assertTrue(output.contains("hello"));
+ } catch (InvocationTargetException t) {
+ Throwable cause = t.getCause();
+ if (cause instanceof IllegalStateException && "Writer Already Closed".equals(cause.getMessage())) {
+ fail("Did not expect a \"Writer Already Closed\" exception");
+ } else {
+ throw t;
+ }
+ }
+ }
+
+
+ /**
+ * Subclass to simulate what the SlingHttpServletResponseImpl writer does
+ */
+ private static final class HandleErrorMockSlingHttpServletResponse extends MockSlingHttpServletResponse {
+ private HandleErrorResponseWriter writer = null;
+
+ @Override
+ public PrintWriter getWriter() {
+ if (writer == null) {
+ writer = new HandleErrorResponseWriter(super.getWriter());
+ }
+ return writer;
+ }
+
+ @Override
+ public void flushBuffer() {
+ if (!writer.isOpen()) {
+ throw new IllegalStateException("Writer Already Closed");
+ }
+ super.flushBuffer();
+ }
+ }
+
+ /**
+ * Subclass to simulate what the SlingHttpServletResponseImpl writer does
+ */
+ private static final class HandleErrorResponseWriter extends PrintWriter {
+
+ private boolean open = true;
+
+ HandleErrorResponseWriter(Writer out) {
+ super(out);
+ }
+
+ @Override
+ public void close() {
+ this.open = false;
+ super.close();
+ }
+
+ public boolean isOpen() {
+ return open;
+ }
+
+ }
+
+}
diff --git a/src/test/java/org/apache/sling/servlets/resolver/it/ServletResolverTestSupport.java b/src/test/java/org/apache/sling/servlets/resolver/it/ServletResolverTestSupport.java
index 54a2269..72f8885 100644
--- a/src/test/java/org/apache/sling/servlets/resolver/it/ServletResolverTestSupport.java
+++ b/src/test/java/org/apache/sling/servlets/resolver/it/ServletResolverTestSupport.java
@@ -75,6 +75,7 @@ public class ServletResolverTestSupport extends TestSupport {
@Configuration
public Option[] configuration() {
+ final String debugPort = System.getProperty("debugPort");
final String vmOpt = System.getProperty("pax.vm.options");
final int httpPort = findFreePort();
versionResolver.setVersionFromProject("org.apache.sling", "org.apache.sling.api");
@@ -84,6 +85,9 @@ public class ServletResolverTestSupport extends TestSupport {
versionResolver.setVersion("org.apache.sling", "org.apache.sling.engine", "2.7.2");
return options(
composite(
+ when(debugPort != null).useOptions(
+ vmOption(String.format("-Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=%s", debugPort))
+ ),
when(vmOpt != null).useOptions(
vmOption(vmOpt)
),
@@ -127,7 +131,7 @@ public class ServletResolverTestSupport extends TestSupport {
}
};
request.setPathInfo(path);
- final MockSlingHttpServletResponse response = new MockSlingHttpServletResponse();
+ final MockSlingHttpServletResponse response = createMockSlingHttpServletResponse();
// Get SlingRequestProcessor.processRequest method and execute request
// This module depends on an older version of the sling.engine module and I don't want
@@ -157,6 +161,10 @@ public class ServletResolverTestSupport extends TestSupport {
return response;
}
+ protected MockSlingHttpServletResponse createMockSlingHttpServletResponse() {
+ return new MockSlingHttpServletResponse();
+ }
+
protected void assertTestServlet(final String path, int expectedStatus) throws Exception {
assertTestServlet(M_GET, path, expectedStatus);
}