You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2006/01/11 00:06:26 UTC
svn commit: r367826 - in /tomcat/container/tc5.5.x:
catalina/src/share/org/apache/catalina/connector/Response.java
webapps/docs/changelog.xml
Author: markt
Date: Tue Jan 10 15:06:22 2006
New Revision: 367826
URL: http://svn.apache.org/viewcvs?rev=367826&view=rev
Log:
Alternative patch for bug 29214 based on Remy's comments
Modified:
tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/connector/Response.java
tomcat/container/tc5.5.x/webapps/docs/changelog.xml
Modified: tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/connector/Response.java
URL: http://svn.apache.org/viewcvs/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/connector/Response.java?rev=367826&r1=367825&r2=367826&view=diff
==============================================================================
--- tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/connector/Response.java (original)
+++ tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/connector/Response.java Tue Jan 10 15:06:22 2006
@@ -219,9 +219,9 @@
private boolean isCharacterEncodingSet = false;
/**
- * The contextType flag
+ * The contentLength flag
*/
- private boolean isContentTypeSet = false;
+ private boolean isContentLengthSet = false;
/**
@@ -275,7 +275,7 @@
appCommitted = false;
included = false;
error = false;
- isContentTypeSet = false;
+ isContentLengthSet = false;
isCharacterEncodingSet = false;
cookies.clear();
@@ -651,6 +651,7 @@
coyoteResponse.reset();
outputBuffer.reset();
+ isContentLengthSet = false;
}
@@ -708,6 +709,8 @@
return;
coyoteResponse.setContentLength(length);
+
+ isContentLengthSet = true;
}
@@ -744,7 +747,8 @@
if (index != -1) {
int len = type.length();
index++;
- while (index < len && Character.isSpace(type.charAt(index))) {
+ while (index < len
+ && Character.isWhitespace(type.charAt(index))) {
index++;
}
if (index+7 < len
@@ -760,8 +764,6 @@
}
}
}
-
- isContentTypeSet = true;
}
@@ -1011,6 +1013,12 @@
coyoteResponse.addHeader(name, value);
+ char cc=name.charAt(0);
+ if(cc=='C' || cc=='c') {
+ if(name.equalsIgnoreCase("Content-Length")) {
+ isContentLengthSet = true;
+ }
+ }
}
@@ -1040,6 +1048,20 @@
* @param name Name of the header to check
*/
public boolean containsHeader(String name) {
+ // Need special handling for Content-Type and Content-Length due to
+ // special handling of these in coyoteResponse
+ char cc=name.charAt(0);
+ if(cc=='C' || cc=='c') {
+ if(name.equalsIgnoreCase("Content-Type")) {
+ // Will return null if this has not been set
+ return (coyoteResponse.getContentType() != null);
+ }
+ if(name.equalsIgnoreCase("Content-Length")) {
+ // Can't use null test since this header is an int
+ return isContentLengthSet;
+ }
+ }
+
return coyoteResponse.containsHeader(name);
}
@@ -1268,6 +1290,13 @@
return;
coyoteResponse.setHeader(name, value);
+
+ char cc=name.charAt(0);
+ if(cc=='C' || cc=='c') {
+ if(name.equalsIgnoreCase("Content-Length")) {
+ isContentLengthSet = true;
+ }
+ }
}
Modified: tomcat/container/tc5.5.x/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewcvs/tomcat/container/tc5.5.x/webapps/docs/changelog.xml?rev=367826&r1=367825&r2=367826&view=diff
==============================================================================
--- tomcat/container/tc5.5.x/webapps/docs/changelog.xml (original)
+++ tomcat/container/tc5.5.x/webapps/docs/changelog.xml Tue Jan 10 15:06:22 2006
@@ -25,11 +25,16 @@
<subsection name="Catalina">
<changelog>
<fix>
- <bug>23950</bug>: Context.listBindings() should return objects not references. (markt)
+ <bug>23950</bug>: Context.listBindings() should return objects not
+ references. (markt)
</fix>
<fix>
<bug>38124</bug>: Add support for Windows 20xx when reading environment
variables in CGIServlet. (markt)
+ </fix>
+ <fix>
+ <bug>29214</bug>: response.containsHeader() now returns the correct
+ value for Content-Type and Content-Length headers. (markt)
</fix>
</changelog>
</subsection>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> Mark Thomas wrote:
>> I thought about doing it that way but I wanted to catch the difference
>> between Content-Length defaulting to -1 and the page developer
>> explicitly setting it to -1. It is an edge case I admit but I don't
>> see any harm in covering it given where the implementation now sits.
>
>
> I don't think Content-Length: -1 is a valid value for the header in the
> response. Since the header is treated as special, I also don't think a
> header will be added to the response in that case.
You are right. I was mixing up my specs. Patch to follow.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java
webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
Mark Thomas wrote:
> I thought about doing it that way but I wanted to catch the difference
> between Content-Length defaulting to -1 and the page developer
> explicitly setting it to -1. It is an edge case I admit but I don't
> see any harm in covering it given where the implementation now sits.
I don't think Content-Length: -1 is a valid value for the header in the
response. Since the header is treated as special, I also don't think a
header will be added to the response in that case.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> markt@apache.org wrote:
>
>> Author: markt
>> Date: Tue Jan 10 15:06:22 2006
>> New Revision: 367826
>>
>> URL: http://svn.apache.org/viewcvs?rev=367826&view=rev
>> Log:
>> Alternative patch for bug 29214 based on Remy's comments
>
>
> I didn't have time to look into it much, but I would have thought using
> coyoteResponse.getContentLength() != -1 would be more good enough.
> Otherwise, the code isn't really that much simpler.
I thought about doing it that way but I wanted to catch the difference
between Content-Length defaulting to -1 and the page developer
explicitly setting it to -1. It is an edge case I admit but I don't
see any harm in covering it given where the implementation now sits.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java webapps/docs/changelog.xml
Posted by Costin Manolache <co...@gmail.com>.
On 1/10/06, Mark Thomas <ma...@apache.org> wrote:
> Costin Manolache wrote:
> > I guess the better solutions are to either deal with this in the
> > servlet facade ( i.e. the HttpServletResponse impl ) - or to just
> > remove the cached fields from coyote Response.
> >
> > The second is harder - but the right one IMO, it's bad to have methods
> > in Response that don't do what people would expect them to do and
> > workaround in facade to deal with complexity. And we already have a
> > cache ( for converted int values ) in MessageBytes.
> >
> > I'm sure Remy would be happier with the first solution, and it's
> > simpler to implement. But I agree that we shouldn't add this to
> > Response.
> >
> > Costin
>
> Given we are working around the cached fields in coyote response, I
> wanted to keep the work around as close to coyote response as possible.
>
> I agree removing the cached fields is the right way to go if we can. I
> did take a quick look at it but was concerned about possible
> performance impacts and wanted to get a better fix out sooner that I
> would have been able to otherwise.
The quickest fix is on the facade ( servlet implementation ), as Remy suggested.
> A related concern was a lack of a test case to check any performance
> impact. I'll put together a simple JMeter test for this. Given that we
> are concerned about request overhead, does the following test case
> sound reasonable?
> - simple servlet (approx 1K output)
Since you want to test the header - probably a small output would be better.
> - explicitly set content length and type in servlet
> - JMeter test case that uses 50 concurrent threads to make 10000 requests
I would test this kind of change with a larger thread count ( 100 + ).
I usually use "ab", not sure how much overhead is in jmeter.
> - set min threads on the connector to >50
> - measure average time per request
All 3 times matter. Make sure you 'warm up' the server.
BTW - I was suggesting to cache instead the MessageBytes corresponding
to the value in the MimeHeaders. But it may be interesting to find if
just removing the cache completely and having simpler code would have
any impact - there are a lot of surprises when you deal with
performance...
Another way to test that would minimize the tomcat overhead is to use
the connector 'standalone' - i.e. write a simple main() that
instantiates the connector, with a simple Adapter that just generates
the content in the buffer. There is some code in sandbox that does
most of this ( in standalone ). This way you test the raw performance
of coyote, without mapper / servlet API / valves / etc.
Costin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Costin Manolache wrote:
> I guess the better solutions are to either deal with this in the
> servlet facade ( i.e. the HttpServletResponse impl ) - or to just
> remove the cached fields from coyote Response.
>
> The second is harder - but the right one IMO, it's bad to have methods
> in Response that don't do what people would expect them to do and
> workaround in facade to deal with complexity. And we already have a
> cache ( for converted int values ) in MessageBytes.
>
> I'm sure Remy would be happier with the first solution, and it's
> simpler to implement. But I agree that we shouldn't add this to
> Response.
>
> Costin
Given we are working around the cached fields in coyote response, I
wanted to keep the work around as close to coyote response as possible.
I agree removing the cached fields is the right way to go if we can. I
did take a quick look at it but was concerned about possible
performance impacts and wanted to get a better fix out sooner that I
would have been able to otherwise.
A related concern was a lack of a test case to check any performance
impact. I'll put together a simple JMeter test for this. Given that we
are concerned about request overhead, does the following test case
sound reasonable?
- simple servlet (approx 1K output)
- explicitly set content length and type in servlet
- JMeter test case that uses 50 concurrent threads to make 10000 requests
- set min threads on the connector to >50
- measure average time per request
Cheers,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java webapps/docs/changelog.xml
Posted by Costin Manolache <co...@gmail.com>.
I guess the better solutions are to either deal with this in the
servlet facade ( i.e. the HttpServletResponse impl ) - or to just
remove the cached fields from coyote Response.
The second is harder - but the right one IMO, it's bad to have methods
in Response that don't do what people would expect them to do and
workaround in facade to deal with complexity. And we already have a
cache ( for converted int values ) in MessageBytes.
I'm sure Remy would be happier with the first solution, and it's
simpler to implement. But I agree that we shouldn't add this to
Response.
Costin
On 1/10/06, Remy Maucherat <re...@apache.org> wrote:
> markt@apache.org wrote:
> > Author: markt
> > Date: Tue Jan 10 15:06:22 2006
> > New Revision: 367826
> >
> > URL: http://svn.apache.org/viewcvs?rev=367826&view=rev
> > Log:
> > Alternative patch for bug 29214 based on Remy's comments
>
> I didn't have time to look into it much, but I would have thought using
> coyoteResponse.getContentLength() != -1 would be more good enough.
> Otherwise, the code isn't really that much simpler.
>
> Rémy
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java
webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
markt@apache.org wrote:
> Author: markt
> Date: Tue Jan 10 15:06:22 2006
> New Revision: 367826
>
> URL: http://svn.apache.org/viewcvs?rev=367826&view=rev
> Log:
> Alternative patch for bug 29214 based on Remy's comments
I didn't have time to look into it much, but I would have thought using
coyoteResponse.getContentLength() != -1 would be more good enough.
Otherwise, the code isn't really that much simpler.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Henri Gomez wrote:
> What about using JMeter ?
I don't need the sophistication for this test and ab looks to have a
smaller overhead.
> Also your test should be from a different computer, may be with 2/3
> clients computers against one single server.
Thanks for the tip. Time to dig out my old laptop.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java webapps/docs/changelog.xml
Posted by Henri Gomez <he...@gmail.com>.
What about using JMeter ?
Also your test should be from a different computer, may be with 2/3
clients computers against one single server.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java webapps/docs/changelog.xml
Posted by Bill Barker <wb...@wilshire.com>.
> -----Original Message-----
> From: Mark Thomas [mailto:markt@apache.org]
> Sent: Thursday, January 12, 2006 1:52 PM
> To: Tomcat Developers List
> Subject: Re: svn commit: r367826 - in
> /tomcat/container/tc5.5.x:
> catalina/src/share/org/apache/catalina/connector/Response.java
> webapps/docs/changelog.xml
>
> Remy Maucherat wrote:
> > I spent some time eliminating all the warnings that my Eclipse was
> > reporting sometimes in the past, and saw that isSpace was
> deprecated.
> > isWhitespace seemed to be a replacement, but is actually
> different. HTTP
> > needs to stay with the non character encoded version. Ie,
> the deprecated
> > method is actually useful, since it does exactly what HTTP needs.
>
> Thanks for spotting this. I'll revert this bit of clean up.
>
> I have got Costin's standalone connector working and I about to start
> looking a better fix. What would be a sensible ab test to run to check
> for performance gain/loss? The connector is wired up to respond with a
> simple, small hello world page.
>
> I am considering something like:
> ab -n 20000 -c 100 -k http://localhost:8800/
> but would appreciate any advice.
>
> The connector is set up with 100 threads.
>
You want at least 150 threads on the connector, or it will quickly start
denying keep-alives.
> Cheers,
>
> Mark
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
>
This message is intended only for the use of the person(s) listed above as the intended recipient(s), and may contain information that is PRIVILEGED and CONFIDENTIAL. If you are not an intended recipient, you may not read, copy, or distribute this message or any attachment. If you received this communication in error, please notify us immediately by e-mail and then delete all copies of this message and any attachments.
In addition you should be aware that ordinary (unencrypted) e-mail sent through the Internet is not secure. Do not send confidential or sensitive information, such as social security numbers, account numbers, personal identification numbers and passwords, to us via ordinary (unencrypted) e-mail.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
Remy Maucherat wrote:
> I spent some time eliminating all the warnings that my Eclipse was
> reporting sometimes in the past, and saw that isSpace was deprecated.
> isWhitespace seemed to be a replacement, but is actually different. HTTP
> needs to stay with the non character encoded version. Ie, the deprecated
> method is actually useful, since it does exactly what HTTP needs.
Thanks for spotting this. I'll revert this bit of clean up.
I have got Costin's standalone connector working and I about to start
looking a better fix. What would be a sensible ab test to run to check
for performance gain/loss? The connector is wired up to respond with a
simple, small hello world page.
I am considering something like:
ab -n 20000 -c 100 -k http://localhost:8800/
but would appreciate any advice.
The connector is set up with 100 threads.
Cheers,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r367826 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/connector/Response.java
webapps/docs/changelog.xml
Posted by Remy Maucherat <re...@apache.org>.
markt@apache.org wrote:
> Author: markt
> Date: Tue Jan 10 15:06:22 2006
> New Revision: 367826
>
> URL: http://svn.apache.org/viewcvs?rev=367826&view=rev
> Log:
> Alternative patch for bug 29214 based on Remy's comments
I am merging the latest version of the patch in my tree, but this change
is a bit sneaky.
> @@ -744,7 +747,8 @@
> if (index != -1) {
> int len = type.length();
> index++;
> - while (index < len && Character.isSpace(type.charAt(index))) {
> + while (index < len
> + && Character.isWhitespace(type.charAt(index))) {
> index++;
> }
> if (index+7 < len
I spent some time eliminating all the warnings that my Eclipse was
reporting sometimes in the past, and saw that isSpace was deprecated.
isWhitespace seemed to be a replacement, but is actually different. HTTP
needs to stay with the non character encoded version. Ie, the deprecated
method is actually useful, since it does exactly what HTTP needs.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org