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 <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
---------------------------------------------------------------------
To start a new topic, e-mail: users@tomcat.apache.org
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
