tomcat-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Schultz <ch...@christopherschultz.net>
Subject Re: Logging all data sent to client
Date Wed, 25 Jul 2007 16:38:32 GMT
-----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


Mime
View raw message