Mike,
Many thanks for your feedback. I did yet have a chance to look into your
modifications but I surely will
Jeff,
I understand your reluctance to sacrifice performance for a purely
cosmetic improvement. I cant say I disagree.
However, apart from logging issue there's a bit of a problem with the
way headers are parsed right now. Have a look
public static String readLine(InputStream inputStream) throws
IOException {
LOG.trace("enter HttpConnection.readLine()");
StringBuffer buf = new StringBuffer();
int ch = inputStream.read();
while (ch >= 0) {
if (ch == '\r') {
ch = inputStream.read();
if (ch == '\n') {
break;
} else {
buf.append('\r');
}
}
buf.append((char) ch);
ch = inputStream.read();
}
...
return (buf.toString());
}
Firstly, there's end of stream check missing, but it's just a minor bug
ch = inputStream.read();
if (ch == -1) {
break;
}
if (ch == '\n') {
break;
} else {
buf.append('\r');
}
Secondly, I am a bit unhappy with this line
buf.append((char) ch);
that explicitly casts byte to char. Of course, we all know that HTTP
uses ASCII for non-content data such as header and is VEEEEEEEERY
unlikely to change to UTF-8 or anything of a sort.
It's just a question of whether we do things right (even if it means
taking a bit of performance hit) or not. I strongly believe that we
should use byte[] to String (and vise versa) conversion routines
provided by HttpConstans consistently throughout the entire code base,
even though it may require a bit of buffering and hence a bit of
performance hit
Bottom line. Let's tackle problems step by step through a series of
small patches and if we see that wire logging cannot be made
comprehensive, so be it.
Personally I am a strong believer in making things work 100% or not
making them at all. I'd rather remove wire logging all together, or
restrict it to exclusively dealing with request/response headers, rather
than having it occasionally spit out some bits and pieces.
This said, I am perfectly aware that in this world 95 (or 98) percent
solutions seems to be more successful ;-) So, I am not going to start
World War III if the wire logging is left as it is
Cheers
Oleg
On Thu, 2003-02-13 at 07:30, Jeffrey Dever wrote:
> Haven't had time to review all of this, but I am a bit concerned over
> any performance issues and buffering. The wirelog is a good thing, but
> there are other ways of getting a request/response log. I wouldnot like
> to see it excessively effect performance or resident set size, or add
> too much complexity.
>
> Also I find that the wirelog output conforms to the 10%/90% rule where
> 90% of its troubleshooting value comes from 10% of its output, namely
> the headers and request/response status lines. The applications I have
> written based on HttpClient would never enable the wirelog because the
> output was too large (and could contain offensive binary data), so I
> would just never enable it and use the applications logging system and
> my own methods to write out the headers. This should be a better
> service provided by HttpClient.
>
> We could just read/buffer/process/log headers in one shot in one logger,
> and handle the body logging seperately. The header logs might be part
> of the normal operation of some applications where the body log would
> only be used for headscratching debugging. That might help a little bit
> with the seperation of concerns that we are grappling here.
>
> Jandalf.
>
>
> Michael Becke wrote:
>
> > Well after looking at this further it seems one problem with this is
> > that bytes get written one at a time to the input WireLog. I made a
> > few changes in HttpConnection and WireLogInputStream that pose a
> > possible solution. This version of the InputStream buffers content
> > until a "\r\n" is read or until a certain number of bytes are read.
> > This way wire log will always work even if content is read using the
> > socket input stream. Take a look. These are just some other ideas.
> >
> > Mike
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
