tomcat-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ben short" <...@benshort.co.uk>
Subject Re: Logging all data sent to client
Date Wed, 25 Jul 2007 20:19:55 GMT
Hi Christopher,

I ripped and altered the code of the Compression Filter servlet
example from tomcat 5.5.23.

There is no LoggingServletWriter, if you look in the original
getWriter method the ServletOutputStream is wrapped by a PrintWriter.
So your proposed change of the getWriter method to the following would
result in the ServletOutputStream not being used if the client of the
LoggingHttpServletResponse called the getWriter method and thus no
logging of the response.

public PrintWriter getWriter() throws IOException
{
   if(null == mWriter)
        mWriter = new LoggingServletWriter(super.getWriter());

   return mWriter;
}

So saying that the finishResponse method change is not valid also. As
the writer flush method should be called if a writer was created,
which in turn will call the stream it has wrapped flush method.

I agree that the check to see if the logger is enabled in the close
method is not needed, and well spotted about the NPE.

Regards

Ben

On 7/25/07, Christopher Schultz <chris@christopherschultz.net> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Ben,
>
> I might change your LoggingHttpServletResponse slightly. I think it's
> more complicated than necessary.
>
> ben short wrote:
> > LoggingHttpServletResponse.java
> >
> > class LoggingHttpServletResponse extends HttpServletResponseWrapper
> >    {
> >    private Logger mLogger =
> > Logger.getLogger(LoggingHttpServletResponse.class);
> >
> >    public LoggingHttpServletResponse(HttpServletResponse
> > httpServletResponse)
> >        {
> >        super(httpServletResponse);
> >        mWrappedResponse = httpServletResponse;
>
> // I would remove the mWrappedResponse. You can always call
> super.getResponse to get the wrapped response.
>
> >        }
>
> [snip]
>
> >    // --------------------------------------------------------- Public
> > Methods
> >
> >    /**
> >     * Create and return a ServletOutputStream to write the content
> >     * associated with this Response.
> >     *
> >     * @throws IOException if an input/output error occurs
> >     */
> >    public ServletOutputStream createOutputStream() throws IOException
> >        {
> >        mLogger.debug("Creating new LoggingOutputStream");
> >
> >        return new LoggingServletOutputStream(mWrappedResponse, mLogger);
> >        }
>
> Is this method useful? The constructor for LSOS ought to be enough.
>
> >    /**
> >     * Finish a response.
> >     */
> >    public void finishResponse()
> >        {
> >        try
> >            {
> >            if (mWriter != null)
> >                {
> >                mWriter.close();
> >                }
> >            else
> >                {
> >                if (mStream != null)
> >                    mStream.close();
> >                }
> >            }
> >        catch (IOException e)
> >            {
> >            }
> >        }
>
> Same here. You should not need a finishResponse method. You ought to
> allow the 'close' method to do its job. Remove this method.
>
> >    /**
> >     * Flush the buffer and commit this response.
> >     *
> >     * @throws IOException if an input/output error occurs
> >     */
> >    public void flushBuffer() throws IOException
> >        {
> >        mStream.flush();
> >        }
>
> How about:
>
> public void flushBuffer()
>     throws IOException
> {
>     if(null != mStream)
>        mStream.flush();
>     else if(null != mWriter)
>        mWriter.flush();
> }
>
> >    public ServletOutputStream getOutputStream() throws IOException
> >        {
> >        if (mWriter != null)
> >            throw new IllegalStateException("getWriter() has already
> > been called for this response");
> >
> >        if (mStream == null)
> >            mStream = createOutputStream();
> >
> >        mLogger.debug("mStream is set to " + mStream + " in
> > getOutputStream");
> >
> >        return (mStream);
> >        }
>
> This method should be:
>
> public ServletOutputStream getOutputStream()
> {
>     if(null == mStream)
>         mStream = new LoggingServletOutputStream(super.getOutputStream));
>
>     return mStream;
> }
>
> >    public PrintWriter getWriter() throws IOException
> >        {
> >        if (mWriter != null)
> >            return (mWriter);
> >
> >        if (mStream != null)
> >            throw new IllegalStateException("getOutputStream() has
> > already been called for this response");
> >
> >        mStream = createOutputStream();
> >
> >        mLogger.debug("mStream is set to " + mStream + " in
> > getOutputStream");
> >
> >        // HttpServletResponse.getCharacterEncoding() shouldn't return null
> >        // according the spec, so feel free to remove that "if"
> >        mWriter = new PrintWriter(mStream);
> >
> >        return (mWriter);
> >        }
> >    }
>
> Similarly:
>
> public PrintWriter getWriter()
>    throws IOException
> {
>     if(null == mWriter)
>          mWriter = new LoggingServletWriter(super.getWriter());
>
>     return mWriter;
> }
>
>
> > LoggingServletOutputStream.java
> >
> > class LoggingServletOutputStream extends ServletOutputStream
> >    {
> >    private Logger mLogger;
> >    private HttpServletResponse mResponse;
>
> The response is not necessary. Remove this.
>
> >    private OutputStream mOutputStream;
> >    private ByteArrayOutputStream mByteArrayOutputStream = new
> > ByteArrayOutputStream();
>
> Good.
>
> >    public LoggingServletOutputStream(HttpServletResponse response,
> > Logger logger) throws IOException
> >        {
> >        mResponse = response;
> >        mOutputStream = mResponse.getOutputStream();
> >        mLogger = logger;
> >        }
>
> Remove the response object, and replace it with the output stream directly.
>
> >    public void write(int b) throws IOException
> >        {
> >        mByteArrayOutputStream.write(b);
> >        mOutputStream.write(b);
> >        }
> >
> >    @Override
> >    public void write(byte b[]) throws IOException
> >        {
> >        mByteArrayOutputStream.write(b);
> >        mOutputStream.write(b);
> >        }
> >
> >    @Override
> >    public void write(byte b[], int off, int len) throws IOException
> >        {
> >        mByteArrayOutputStream.write(b, off, len);
> >        mOutputStream.write(b, off, len);
> >        }
>
> Good.
>
> >    @Override
> >    public void close() throws IOException
> >        {
> >        if ( mLogger.isDebugEnabled() )
>
> You probably don't need to check for that, here. The response wouldn't
> have been wrapped with the logger if you didn't want the output. It
> doesn't hurt, but it makes the code more complicated and I don't think
> you need it.
>
> >            {
> >            float kBytes = mByteArrayOutputStream.size() / 1024f;
> >
> >            mLogger.debug("Writing " + kBytes + " kb (" +
> > mByteArrayOutputStream.size() +" b) to the client.\n" +
> > mByteArrayOutputStream.toString());
> >            }
> >
> >        mByteArrayOutputStream = null;
> >        mOutputStream.close();
> >        }
>
> I wouldn't set the mByteArrayOutputStream to null, here, just in case
> more data gets written or something. You'd rather have an IOException
> for a closed stream (from mOutputSteam) than a NullPointerException.
>
> You didn't provide an implementation for LoggingServletWriter, but I'm
> sure it's similar to LSOS... I would make the same changes, there.
>
> I hope my comments are useful. Your filter is good, and with my changes
> it's leaner and meaner.
>
> - -chris
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.7 (MingW32)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFGp3yI9CaO5/Lv0PARAuW7AJ4u4jfH43ge8LOegt6OhCYwTm3imgCgsxLR
> uwQNI0TNTlePu8gZPDqkEbY=
> =OUKH
> -----END PGP SIGNATURE-----
>
> ---------------------------------------------------------------------
> To start a new topic, e-mail: users@tomcat.apache.org
> To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: users-help@tomcat.apache.org
>
>

---------------------------------------------------------------------
To start a new topic, e-mail: users@tomcat.apache.org
To unsubscribe, e-mail: users-unsubscribe@tomcat.apache.org
For additional commands, e-mail: users-help@tomcat.apache.org


Mime
View raw message