[ https://issues.apache.org/jira/browse/SLING-7813?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17414837#comment-17414837 ]
Bertrand Delacretaz edited comment on SLING-7813 at 9/14/21, 11:49 AM: ----------------------------------------------------------------------- _Update: the below comment is not valid as it refers to sendError, which does declare exceptions in the Servlet API, but setStatus does not_ I stumbled on this while looking at SLING-10810 and I think [commit bc9696bd|https://github.com/apache/sling-org-apache-sling-engine/commit/bc9696bd4f52a3a2b50a19f2572b80a4ac2ea3a2] introduces a non-standard behavior, where setStatus just logs a message if the request is already committed, instead of throwing an exception like [HttpServletResponse.sendError|https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpServletResponse.html#sendError-int-java.lang.String-] does. Actually, IIUC that non-standard behavior only happens if setStatus(status, message) is called with a non-empty message, otherwise super.setStatus is called. That's suboptimal IMO, but considering that these changes are more than 3 years old now there's probably no urgent need to change that back - I'll just leave that note here in case we want to revisit this in the future. was (Author: bdelacretaz): I stumbled on this while looking at SLING-10810 and I think [commit bc9696bd|https://github.com/apache/sling-org-apache-sling-engine/commit/bc9696bd4f52a3a2b50a19f2572b80a4ac2ea3a2] introduces a non-standard behavior, where setStatus just logs a message if the request is already committed, instead of throwing an exception like [HttpServletResponse.sendError|https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpServletResponse.html#sendError-int-java.lang.String-] does. Actually, IIUC that non-standard behavior only happens if setStatus(status, message) is called with a non-empty message, otherwise super.setStatus is called. That's suboptimal IMO, but considering that these changes are more than 3 years old now there's probably no urgent need to change that back - I'll just leave that note here in case we want to revisit this in the future. > SlingHttpServletResponseImpl should log when setStatus is called after it is > committed > -------------------------------------------------------------------------------------- > > Key: SLING-7813 > URL: https://issues.apache.org/jira/browse/SLING-7813 > Project: Sling > Issue Type: Improvement > Components: Engine > Affects Versions: Engine 2.6.12 > Reporter: Julian Sedding > Assignee: Julian Sedding > Priority: Minor > Fix For: Engine 2.6.14 > > Time Spent: 0.5h > Remaining Estimate: 0h > > I have been debugging a scenario, where a response did not have the (in this > case) expected status code {{500}} set by an error handling script, but > instead the status code was {{200}}. > It turns out that a rendering script calls {{#flushBuffer()}} on the response > early on in order to optimize user experience. Later in the rendering chain a > JSP causes a {{NullPointerException}}, triggering an error handler which > calls {{#setStatus(500)}}. The {{#setStatus}} call is silently ignored. > "Fixing" this problem would require buffering the entire response and > ignoring any flush calls (be it {{#flushBuffer()}}, {{#getWriter().flush()}} > or {{#getOutputStream().flush()}}). This would be a change in behaviour, a > violation of the Servlet spec and performance issues waiting to happen. Thus > I am ruling out this option. > However, it would be helpful to improve "debuggability" of the problem. I > propose to log a warning when {{#setStatus()}} is called. Additionally, if > debug logging is enabled, I propose to log a stack trace to identify where > the flush call originated (unless the flush was due to too many bytes > written, which is not very helpful information). > cc [~rombert] -- This message was sent by Atlassian Jira (v8.3.4#803005)