From commits-return-18487-archive-asf-public=cust-asf.ponee.io@struts.apache.org Tue Oct 1 06:37:13 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 0BF19180608 for ; Tue, 1 Oct 2019 08:37:12 +0200 (CEST) Received: (qmail 73731 invoked by uid 500); 1 Oct 2019 06:37:12 -0000 Mailing-List: contact commits-help@struts.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@struts.apache.org Delivered-To: mailing list commits@struts.apache.org Received: (qmail 73722 invoked by uid 99); 1 Oct 2019 06:37:12 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 01 Oct 2019 06:37:12 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 31CC681E87; Tue, 1 Oct 2019 06:37:12 +0000 (UTC) Date: Tue, 01 Oct 2019 06:37:12 +0000 To: "commits@struts.apache.org" Subject: [struts] branch master updated: Merge pull request #366 from JCgH4164838Gh792C124B5/local_25x_SendErrorEnh MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Message-ID: <156991183198.27171.16033072312163208299@gitbox.apache.org> From: lukaszlenart@apache.org X-Git-Host: gitbox.apache.org X-Git-Repo: struts X-Git-Refname: refs/heads/master X-Git-Reftype: branch X-Git-Oldrev: ad864ac9983ee05976e5b02231face4e368ec1ae X-Git-Newrev: 1a93a300bb75184179bc223441d9d6684f0434f1 X-Git-Rev: 1a93a300bb75184179bc223441d9d6684f0434f1 X-Git-NotificationType: ref_changed_plus_diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/struts.git The following commit(s) were added to refs/heads/master by this push: new 1a93a30 Merge pull request #366 from JCgH4164838Gh792C124B5/local_25x_SendErrorEnh new 544c353 Merge pull request #369 from JCgH4164838Gh792C124B5/local_26x_CPickPR366 1a93a30 is described below commit 1a93a300bb75184179bc223441d9d6684f0434f1 Author: Lukasz Lenart AuthorDate: Mon Sep 23 08:54:02 2019 +0200 Merge pull request #366 from JCgH4164838Gh792C124B5/local_25x_SendErrorEnh Improved logging for DefaultDispatcherErrorHandler, DefaultStaticContentLoader (cherry picked from commit 12d4feaf8f90b3af3853eb53263410e8e6f1e956) --- .../dispatcher/DefaultDispatcherErrorHandler.java | 8 ++ .../dispatcher/DefaultStaticContentLoader.java | 10 +- .../DefaultDispatcherErrorHandlerTest.java | 138 +++++++++++++++++++++ .../dispatcher/DefaultStaticContentLoaderTest.java | 62 +++++++++ 4 files changed, 217 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java b/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java index 0769771..551ff1e 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandler.java @@ -97,6 +97,10 @@ public class DefaultDispatcherErrorHandler implements DispatcherErrorHandler { response.sendError(code, e.getMessage()); } catch (IOException e1) { // we're already sending an error, not much else we can do if more stuff breaks + LOG.warn("Unable to send error response, code: {};", code, e1); + } catch (IllegalStateException ise) { + // Log illegalstate instead of passing unrecoverable exception to calling thread + LOG.warn("Unable to send error response, code: {}; isCommited: {};", code, response.isCommitted(), ise); } } @@ -122,6 +126,10 @@ public class DefaultDispatcherErrorHandler implements DispatcherErrorHandler { response.sendError(code, "Unable to show problem report:\n" + exp + "\n\n" + LocationUtils.getLocation(exp)); } catch (IOException ex) { // we're already sending an error, not much else we can do if more stuff breaks + LOG.warn("Unable to send error response, code: {};", code, ex); + } catch (IllegalStateException ise) { + // Log illegalstate instead of passing unrecoverable exception to calling thread + LOG.warn("Unable to send error response, code: {}; isCommited: {};", code, response.isCommitted(), ise); } } } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java b/core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java index eeedbf5..42f1716 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/DefaultStaticContentLoader.java @@ -218,7 +218,15 @@ public class DefaultStaticContentLoader implements StaticContentLoader { } } - response.sendError(HttpServletResponse.SC_NOT_FOUND); + try { + response.sendError(HttpServletResponse.SC_NOT_FOUND); + } catch (IOException e1) { + // we're already sending an error, not much else we can do if more stuff breaks + LOG.warn("Unable to send error response, code: {};", HttpServletResponse.SC_NOT_FOUND, e1); + } catch (IllegalStateException ise) { + // Log illegalstate instead of passing unrecoverable exception to calling thread + LOG.warn("Unable to send error response, code: {}; isCommited: {};", HttpServletResponse.SC_NOT_FOUND, response.isCommitted(), ise); + } } protected void process(InputStream is, String path, HttpServletRequest request, HttpServletResponse response) throws IOException { diff --git a/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java b/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java new file mode 100644 index 0000000..e89dd39 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/DefaultDispatcherErrorHandlerTest.java @@ -0,0 +1,138 @@ +/* + * 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.struts2.dispatcher; + +import java.io.IOException; +import java.util.Collections; +import org.apache.struts2.StrutsInternalTestCase; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.apache.struts2.views.freemarker.FreemarkerManager; +import static org.easymock.EasyMock.anyInt; +import static org.easymock.EasyMock.anyString; +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.replay; + +public class DefaultDispatcherErrorHandlerTest extends StrutsInternalTestCase { + private HttpServletRequest requestMock; + private HttpServletResponse responseMock; + + /** + * Test to exercise the code path and prove handleError() will output + * the desired log warning when an IOException is thrown with devMode false. + */ + public void testHandleErrorIOException() { + DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new DefaultDispatcherErrorHandler(); + defaultDispatcherErrorHandler.setDevMode("false"); + defaultDispatcherErrorHandler.setFreemarkerManager(dispatcher.getContainer().getInstance(FreemarkerManager.class)); + defaultDispatcherErrorHandler.init(dispatcher.servletContext); + Exception fakeException = new Exception("Fake Exception, devMode false"); + try { + requestMock.setAttribute("javax.servlet.error.exception", fakeException); + expectLastCall(); + requestMock.setAttribute("javax.servlet.jsp.jspException", fakeException); + expectLastCall(); + responseMock.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException.getMessage()); + expectLastCall().andStubThrow(new IOException("Fake IO Exception (SC_INTERNAL_SERVER_ERROR, devMode false)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + defaultDispatcherErrorHandler.handleError(requestMock, responseMock, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException); + } + + /** + * Test to exercise the code path and prove handleError() will output + * the desired log warning when an IOException is thrown with devMode true. + */ + public void testHandleErrorIOExceptionDevMode() { + DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new DefaultDispatcherErrorHandler(); + defaultDispatcherErrorHandler.setDevMode("true"); + defaultDispatcherErrorHandler.setFreemarkerManager(dispatcher.getContainer().getInstance(FreemarkerManager.class)); + defaultDispatcherErrorHandler.init(dispatcher.servletContext); + Exception fakeException = new Exception("Fake Exception, devMode true"); + try { + responseMock.setContentType("text/html"); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (report write)")); // Fake error during report write + responseMock.sendError(anyInt(), anyString()); + expectLastCall().andStubThrow(new IOException("Fake IO Exception (SC_INTERNAL_SERVER_ERROR, devMode true)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + defaultDispatcherErrorHandler.handleError(requestMock, responseMock, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException); + } + + /** + * Test to exercise the code path and prove handleError() will output + * the desired log warning when an IllegalStateException is thrown with devMode false. + */ + public void testHandleErrorIllegalStateException() { + DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new DefaultDispatcherErrorHandler(); + defaultDispatcherErrorHandler.setDevMode("false"); + defaultDispatcherErrorHandler.setFreemarkerManager(dispatcher.getContainer().getInstance(FreemarkerManager.class)); + defaultDispatcherErrorHandler.init(dispatcher.servletContext); + Exception fakeException = new Exception("Fake Exception, devMode false"); + try { + requestMock.setAttribute("javax.servlet.error.exception", fakeException); + expectLastCall(); + requestMock.setAttribute("javax.servlet.jsp.jspException", fakeException); + expectLastCall(); + expect(responseMock.isCommitted()).andStubReturn(Boolean.TRUE); + responseMock.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException.getMessage()); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (SC_INTERNAL_SERVER_ERROR, devMode false)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + defaultDispatcherErrorHandler.handleError(requestMock, responseMock, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException); + } + + /** + * Test to exercise the code path and prove handleError() will output + * the desired log warning when an IllegalStateException is thrown with devMode true. + */ + public void testHandleErrorIllegalStateExceptionDevMode() { + DefaultDispatcherErrorHandler defaultDispatcherErrorHandler = new DefaultDispatcherErrorHandler(); + defaultDispatcherErrorHandler.setDevMode("true"); + defaultDispatcherErrorHandler.setFreemarkerManager(dispatcher.getContainer().getInstance(FreemarkerManager.class)); + defaultDispatcherErrorHandler.init(dispatcher.servletContext); + Exception fakeException = new Exception("Fake Exception, devMode true"); + try { + expect(responseMock.isCommitted()).andStubReturn(Boolean.TRUE); + responseMock.setContentType("text/html"); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (report write)")); // Fake error during report write + responseMock.sendError(anyInt(), anyString()); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (SC_INTERNAL_SERVER_ERROR, devMode true)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + defaultDispatcherErrorHandler.handleError(requestMock, responseMock, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, fakeException); + } + + protected void setUp() { + requestMock = (HttpServletRequest) createMock(HttpServletRequest.class); + responseMock = (HttpServletResponse) createMock(HttpServletResponse.class); + dispatcher = initDispatcher(Collections.emptyMap()); + } +} diff --git a/core/src/test/java/org/apache/struts2/dispatcher/DefaultStaticContentLoaderTest.java b/core/src/test/java/org/apache/struts2/dispatcher/DefaultStaticContentLoaderTest.java index e8d077d..8f8eda1 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/DefaultStaticContentLoaderTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/DefaultStaticContentLoaderTest.java @@ -18,11 +18,22 @@ */ package org.apache.struts2.dispatcher; +import java.io.IOException; import org.apache.struts2.StrutsInternalTestCase; import java.util.List; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import static org.easymock.EasyMock.createMock; +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.EasyMock.replay; public class DefaultStaticContentLoaderTest extends StrutsInternalTestCase { + private HttpServletRequest requestMock; + private HttpServletResponse responseMock; + private HostConfig hostConfigMock; + private DefaultStaticContentLoader defaultStaticContentLoader; public void testParsePackages() throws Exception { @@ -50,4 +61,55 @@ public class DefaultStaticContentLoaderTest extends StrutsInternalTestCase { assertEquals(result4.get(3), "foo/bar/package4/"); } + /** + * Test to exercise the code path and prove findStaticResource() will output + * the desired log warning when an IOException is thrown. + */ + public void testFindStaticResourceIOException() { + expect(requestMock.getDateHeader("If-Modified-Since")).andStubReturn(0L); + try { + responseMock.sendError(HttpServletResponse.SC_NOT_FOUND); + expectLastCall().andStubThrow(new IOException("Fake IO Exception (SC_NOT_FOUND)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + try { + defaultStaticContentLoader.findStaticResource("/static/fake.html", requestMock, responseMock); + } catch (IOException ioe) { + fail("DefaultStaticContentLoader.findStaticResource() call failed. Ex: " + ioe); + } + } + + /** + * Test to exercise the code path and prove findStaticResource() will output + * the desired log warning when an IllegalStateException is thrown. + */ + public void testFindStaticResourceIllegalStateException() { + expect(requestMock.getDateHeader("If-Modified-Since")).andStubReturn(0L); + try { + expect(responseMock.isCommitted()).andStubReturn(Boolean.TRUE); + responseMock.sendError(HttpServletResponse.SC_NOT_FOUND); + expectLastCall().andStubThrow(new IllegalStateException("Fake IllegalState Exception (SC_NOT_FOUND)")); + replay(responseMock); + } catch (IOException ioe) { + fail("Mock sendError call setup failed. Ex: " + ioe); + } + try { + defaultStaticContentLoader.findStaticResource("/static/fake.html", requestMock, responseMock); + } catch (IOException ioe) { + fail("DefaultStaticContentLoader.findStaticResource() call failed. Ex: " + ioe); + } + } + + protected void setUp() { + requestMock = (HttpServletRequest) createMock(HttpServletRequest.class); + responseMock = (HttpServletResponse) createMock(HttpServletResponse.class); + hostConfigMock = (HostConfig) createMock(HostConfig.class); + expect(hostConfigMock.getInitParameter("packages")).andStubReturn(null); + expect(hostConfigMock.getInitParameter("loggerFactory")).andStubReturn(null); + defaultStaticContentLoader = new DefaultStaticContentLoader(); + defaultStaticContentLoader.setHostConfig(hostConfigMock); + defaultStaticContentLoader.setEncoding("UTF-8"); + } }