cocoon-cvs mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From gkossakow...@apache.org
Subject svn commit: r685544 - in /cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src: changes/ main/java/org/apache/cocoon/servletservice/ test/java/org/apache/cocoon/servletservice/
Date Wed, 13 Aug 2008 13:29:47 GMT
Author: gkossakowski
Date: Wed Aug 13 06:29:46 2008
New Revision: 685544

URL: http://svn.apache.org/viewvc?rev=685544&view=rev
Log:
Reworked HttpServletResponeBufferingWrapper a little bit.

The main idea behind all my changes is to move decision about buffering or not directly to
our own implementation of ServletOutputStream.
ForwardingOrLimitingServletOutputStream acts in two modes depending on setting if it should
buffer or just forward.
This change was needed because there might be following execution of methods for which previous
implementation would fail:
1. stream = response.getOutputStream() //non buffering output stream is returned because by
default status code is set to 200
2. response.setStatusCode(404)
3. stream.write() //write details about error, this is going to be written to response because
non-buffering output stream is in use
4. response.resetBufferedResponse() //this fails with IllegalStateException

This problem was recorded in COCOON-2337 and now is fixed.

Also, test-case covering this problem was added and implementation of existing ones has been
changed a little bit to more carefully check if buffering really works as expected.

Modified:
    cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/changes/changes.xml
    cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapper.java
    cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/test/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapperTestCase.java

Modified: cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/changes/changes.xml
URL: http://svn.apache.org/viewvc/cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/changes/changes.xml?rev=685544&r1=685543&r2=685544&view=diff
==============================================================================
--- cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/changes/changes.xml
(original)
+++ cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/changes/changes.xml
Wed Aug 13 06:29:46 2008
@@ -24,6 +24,12 @@
 -->
 <document>
   <body>
+    <release version="1.1.1-SNAPSHOT" date="2008-??-??" description="unreleased">
+      <action dev="gkossakowski" type="fix" issue="COCOON-2237">
+        Fixed HttpServletResponseBufferingWrapper implementation so it does not throw IllegalStateException
+        when resestBufferedResponse is being called.
+      </action>
+    </release>
     <release version="1.1.0" date="2008-08-09" description="released">
       <action dev="reinhard" type="fix">
         The dependency on the Cocoon SourceResolver, that introduced a circular dependency
with Cocoon 2.2, was

Modified: cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapper.java
URL: http://svn.apache.org/viewvc/cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapper.java?rev=685544&r1=685543&r2=685544&view=diff
==============================================================================
--- cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapper.java
(original)
+++ cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/main/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapper.java
Wed Aug 13 06:29:46 2008
@@ -61,7 +61,7 @@
     private int statusCode;
     private boolean sendError;
 
-    private LimitingServletOutputStream outputStream = new LimitingServletOutputStream(BUFFER_LIMIT);
+    private ForwardingOrLimitingServletOutputStream outputStream;
     private PrintWriter printWriter;
 
     public HttpServletResponseBufferingWrapper(HttpServletResponse response) {
@@ -162,26 +162,18 @@
     }
 
     public ServletOutputStream getOutputStream() throws IOException {
-        if (!bufferResponse) {
-            return super.getOutputStream();
-        } else {
-            committed = true;
-            return outputStream;
-        }
+        if (outputStream == null)
+            this.outputStream = new ForwardingOrLimitingServletOutputStream(BUFFER_LIMIT,
super.getOutputStream());
+        return outputStream;
     }
 
     public PrintWriter getWriter() throws IOException {
-        if (!bufferResponse)
-            return super.getWriter();
-        else {
-            if (this.outputStream != null)
-                throw new IllegalStateException(
-                        "Output buffer has been already obtained. You can use either output
buffer or print writer at one time.");
-            if (this.printWriter == null)
-                this.printWriter = new PrintWriter(new OutputStreamWriter(
-                        getOutputStream(), getCharacterEncoding()));
-            return printWriter;
-        }
+        if (this.outputStream != null)
+            throw new IllegalStateException(
+                    "Output buffer has been already obtained. You can use either output buffer
or print writer at one time.");
+        if (this.printWriter == null)
+            this.printWriter = new PrintWriter(new OutputStreamWriter(getOutputStream(),
getCharacterEncoding()));
+        return printWriter;
     }
 
     public void flushBuffer() throws IOException {
@@ -207,6 +199,7 @@
     public void reset() {
         if (isCommitted())
             throw new IllegalStateException(ALREADY_COMMITTED_EXCEPTION);
+        super.reset();
         bufferResponse = false;
         message = null;
     }
@@ -239,8 +232,9 @@
                             "Error occured while writing to printWriter.");
                 this.printWriter.close();
             }
-
-            outputStream.writeTo(super.getOutputStream());
+            
+            if (outputStream != null)
+                outputStream.writeTo(super.getOutputStream());
         }
     }
 
@@ -260,31 +254,40 @@
     }
 
     /**
-     * Simple class acting like a {@link ServletOutputStream} but limiting number of bytes
that can be written to the 
-     * stream.
+     * Simple class acting like a {@link ServletOutputStream} but limiting (if it does not
forward) number of bytes that 
+     * can be written to the stream.
      */
-    private class LimitingServletOutputStream extends ServletOutputStream {
+    private class ForwardingOrLimitingServletOutputStream extends ServletOutputStream {
         
         private Log log = LogFactory.getLog(getClass());
 
         private int writeLimit;
         private ByteArrayOutputStream outputStream;
+        
+        private OutputStream forwardTo;
 
-        public LimitingServletOutputStream(int writeLimit) {
+        public ForwardingOrLimitingServletOutputStream(int writeLimit, OutputStream forwardTo)
{
             this.writeLimit = writeLimit;
+            this.forwardTo = forwardTo;
             reset();
         }
 
         public void write(int b) throws IOException {
-            if (this.outputStream.size() < this.writeLimit)
-                this.outputStream.write(b);
+            HttpServletResponseBufferingWrapper.this.committed = true;
+            
+            if (isForwarding())
+                forwardTo.write(b);
             else {
-                RuntimeException e = new RuntimeException(
-                        "The buffering limit (" + writeLimit+ ") has been reached. If you
encounter this exception it means that you to "
-                        + "write a big response body for response that has error code set
as status code. This is always a bad "
-                        + "idea and in such case you should reconsider your design.");
-                log.fatal("Fatal error occured in writing to response", e);
-                throw e;
+                if (this.outputStream.size() < this.writeLimit)
+                    this.outputStream.write(b);
+                else {
+                    RuntimeException e = new RuntimeException(
+                            "The buffering limit (" + writeLimit+ ") has been reached. If
you encounter this exception it means that you to "
+                            + "write a big response body for response that has error code
set as status code. This is always a bad "
+                            + "idea and in such case you should reconsider your design.");
+                    log.fatal("Fatal error occured in writing to response", e);
+                    throw e;
+                }
             }
         }
 
@@ -295,6 +298,10 @@
         public void writeTo(OutputStream outputStream) throws IOException {
             this.outputStream.writeTo(outputStream);
         }
+        
+        private boolean isForwarding() {
+            return !HttpServletResponseBufferingWrapper.this.bufferResponse;
+        }
 
     }
 

Modified: cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/test/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapperTestCase.java
URL: http://svn.apache.org/viewvc/cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/test/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapperTestCase.java?rev=685544&r1=685543&r2=685544&view=diff
==============================================================================
--- cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/test/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapperTestCase.java
(original)
+++ cocoon/trunk/subprojects/cocoon-servlet-service/cocoon-servlet-service-impl/src/test/java/org/apache/cocoon/servletservice/HttpServletResponseBufferingWrapperTestCase.java
Wed Aug 13 06:29:46 2008
@@ -53,31 +53,43 @@
     }
     
     public void testNoBuffering() throws IOException {
+        CountingServletOutputStream countingStream = new CountingServletOutputStream();
+        
         MockControl control = MockControl.createControl(HttpServletResponse.class);
         HttpServletResponse response = (HttpServletResponse)control.getMock();
         response.setStatus(HttpServletResponse.SC_OK);
         response.isCommitted();
         control.setReturnValue(false, MockControl.ZERO_OR_MORE);
         response.getOutputStream();
-        control.setReturnValue(new ServletOutputStream() {
-            public void write(int b) throws IOException { }
-        });
+        control.setReturnValue(countingStream);
         control.replay();
+        
         HttpServletResponseBufferingWrapper responseWrapper = new HttpServletResponseBufferingWrapper(response);
         responseWrapper.setStatus(HttpServletResponse.SC_OK);
-        responseWrapper.getOutputStream();
+        OutputStream outputStream = responseWrapper.getOutputStream();
+        outputStream.write(0);
+        
+        assertEquals(1, countingStream.getCounter());
         control.verify();
     }
     
     public void testBuffering() throws IOException {
+        CountingServletOutputStream countingStream = new CountingServletOutputStream();
+        
         MockControl control = MockControl.createControl(HttpServletResponse.class);
         HttpServletResponse response = (HttpServletResponse)control.getMock();
         response.isCommitted();
         control.setReturnValue(false, MockControl.ZERO_OR_MORE);
+        response.getOutputStream();
+        control.setReturnValue(countingStream);
         control.replay();
+        
         HttpServletResponseBufferingWrapper responseWrapper = new HttpServletResponseBufferingWrapper(response);
         responseWrapper.setStatus(HttpServletResponse.SC_NOT_FOUND);
-        responseWrapper.getOutputStream();
+        OutputStream outputStream = responseWrapper.getOutputStream();
+        outputStream.write(0);
+        
+        assertEquals(0, countingStream.getCounter());
         control.verify();
     }
     
@@ -86,6 +98,8 @@
         HttpServletResponse response = (HttpServletResponse)control.getMock();
         response.isCommitted();
         control.setReturnValue(false, MockControl.ZERO_OR_MORE);
+        response.getOutputStream();
+        control.setReturnValue(new CountingServletOutputStream());
         control.replay();
         HttpServletResponseBufferingWrapper responseWrapper = new HttpServletResponseBufferingWrapper(response);
         responseWrapper.setStatus(HttpServletResponse.SC_NOT_FOUND);
@@ -113,10 +127,6 @@
         response.isCommitted();
         control.setReturnValue(false, MockControl.ZERO_OR_MORE);
         response.setStatus(HttpServletResponse.SC_NOT_FOUND);
-        response.getOutputStream();
-        control.setReturnValue(new ServletOutputStream() {
-            public void write(int arg0) throws IOException { }
-        });
         control.replay();
         HttpServletResponseBufferingWrapper responseWrapper = new HttpServletResponseBufferingWrapper(response);
         responseWrapper.setStatus(HttpServletResponse.SC_NOT_FOUND);
@@ -124,4 +134,47 @@
         control.verify();
     }
     
+    /**
+     * This method tests late setting of status code to 404. Late here means <b>after</b>
getOutputStream method was called.
+     * @throws IOException
+     */
+    public void testLateSettingStatusCodeTo404() throws IOException {
+        CountingServletOutputStream countingStream = new CountingServletOutputStream();
+        
+        MockControl control = MockControl.createControl(HttpServletResponse.class);
+        HttpServletResponse response = (HttpServletResponse)control.getMock();
+        response.isCommitted();
+        control.setReturnValue(false, MockControl.ZERO_OR_MORE);
+        response.getOutputStream();
+        control.setReturnValue(countingStream);
+        control.replay();
+        
+        HttpServletResponseBufferingWrapper responseWrapper = new HttpServletResponseBufferingWrapper(response);
+        responseWrapper.setStatus(HttpServletResponse.SC_NOT_FOUND);
+        OutputStream outputStream = responseWrapper.getOutputStream();
+        outputStream.write(0);
+        
+        assertEquals(0, countingStream.getCounter());
+        responseWrapper.resetBufferedResponse();
+        assertEquals(0, countingStream.getCounter());
+        control.verify();
+    }
+    
+    /**
+     * Simple ServletOutputStream that counts how many bytes were written.
+     *
+     */
+    private class CountingServletOutputStream extends ServletOutputStream {
+        
+        private int counter = 0;
+
+        public void write(int arg0) throws IOException {
+            counter++;
+        }
+        
+        public int getCounter() {
+            return counter;
+        }
+    }
+    
 }



Mime
View raw message