[TC4] ResourcesBase.setCheckInterval() bug
Hi guys!
In reading through the org.apache.catalina.resources package code, I found
a small omission from the ResourcesBase.setCheckInterval() method:
public void setCheckInterval(int checkInterval) {
// Perform the property update
int oldCheckInterval = this.checkInterval;
this.checkInterval = checkInterval;
support.firePropertyChange("checkInterval",
new Integer(oldCheckInterval),
new Integer(this.checkInterval));
// Start or stop the background thread (if necessary)
if (started) {
if ((oldCheckInterval 0) (this.checkInterval = 0))
threadStop();
else if ((oldCheckInterval = 0) (this.checkInterval 0))
threadStart();
}
}
At the end of this method, we changed the check interval, and then we
need to either start or stop the background thread that periodically
checks for resource updates. The code in this method handles the
following:
1. When the background thread is already running and we should be shutting
it down because the new check interval doesn't require a background
thread at all.
2. When the thread is supposedly already running, the old check interval
didn't require a background thread (really, an illegal state -- and
since
this code looks basically like my patch below, was this just bad
placement
of this code?), and the new check interval does require a background
thread.. In that case the code at least makes sure that we have
a reference to a thread.
But, what it doesn't handle is:
3. When the background thread isn't already running, the previous check
interval didn't require a background thread, and the new check interval
does require a background thread.. It should start one.
So, here's the patch I'd suggest:
280a281,284
else {
if ((oldCheckInterval = 0) (this.checkInterval 0))
threadStart();
}
Thanks!
--
Jason Brittain (415)354-6645
Software Engineer, Olliance Inc.http://www.Olliance.com
Current Maintainer, Locomotive Project http://www.Locomotive.org
Re: [TC4] ResourcesBase.setCheckInterval() bug
At the end of this method, we changed the check interval, and then we
need to either start or stop the background thread that periodically
checks for resource updates. The code in this method handles the
following:
1. When the background thread is already running and we should be shutting
it down because the new check interval doesn't require a background
thread at all.
2. When the thread is supposedly already running, the old check interval
didn't require a background thread (really, an illegal state -- and
since
this code looks basically like my patch below, was this just bad
placement
of this code?), and the new check interval does require a background
thread.. In that case the code at least makes sure that we have
a reference to a thread.
But, what it doesn't handle is:
3. When the background thread isn't already running, the previous check
interval didn't require a background thread, and the new check interval
does require a background thread.. It should start one.
So, here's the patch I'd suggest:
280a281,284
else {
if ((oldCheckInterval = 0) (this.checkInterval 0))
threadStart();
}
The "started" flag indicates that the component has been started. It's
related to the thread state since the thread cannot be started if the
started flag is not set to true. The flag is set by the start() and stop()
method. If the component hasn't been started yet, I don't think it should
start the thread (or try to stop it).
Does it make sense (or did I miss something ?) ?
Remy (going back to optimizing the HTTP connector)
Re: [TC4] ResourcesBase.setCheckInterval() bug
Hi Jason,
See below.
Jason Brittain wrote:
Hi guys!
In reading through the org.apache.catalina.resources package code, I found
a small omission from the ResourcesBase.setCheckInterval() method:
public void setCheckInterval(int checkInterval) {
// Perform the property update
int oldCheckInterval = this.checkInterval;
this.checkInterval = checkInterval;
support.firePropertyChange("checkInterval",
new Integer(oldCheckInterval),
new Integer(this.checkInterval));
// Start or stop the background thread (if necessary)
if (started) {
if ((oldCheckInterval 0) (this.checkInterval = 0))
threadStop();
else if ((oldCheckInterval = 0) (this.checkInterval 0))
threadStart();
}
}
At the end of this method, we changed the check interval, and then we
need to either start or stop the background thread that periodically
checks for resource updates. The code in this method handles the
following:
1. When the background thread is already running and we should be shutting
it down because the new check interval doesn't require a background
thread at all.
2. When the thread is supposedly already running, the old check interval
didn't require a background thread (really, an illegal state -- and
since
this code looks basically like my patch below, was this just bad
placement
of this code?), and the new check interval does require a background
thread.. In that case the code at least makes sure that we have
a reference to a thread.
But, what it doesn't handle is:
3. When the background thread isn't already running, the previous check
interval didn't require a background thread, and the new check interval
does require a background thread.. It should start one.
So, here's the patch I'd suggest:
280a281,284
else {
if ((oldCheckInterval = 0) (this.checkInterval 0))
threadStart();
}
You're correct that this kind of code is appropriate (because the component
has already been started without the thread). What's puzzling me is that my
copy of ResourcesBase.java (version 1.3, last updated 2000/10/17) already
includes code very similar to this. Checking the CVS logs, it seems that this
code has been there at least since we moved Catalina over to the new
"jakarta-tomcat-4.0" CVS repository in August.
What version of Tomcat 4.0 sources are you looking at?
Thanks!
--
Jason Brittain (415)354-6645
Craig
Re: [TC4] ResourcesBase.setCheckInterval() bug
You're correct that this kind of code is appropriate (because the component has already been started without the thread). Really ? The threadStart() call is in the start() method, and threadStop() is always called in stop(). How would the thread need to be started if the component is not started yet ? What's puzzling me is that my copy of ResourcesBase.java (version 1.3, last updated 2000/10/17) already includes code very similar to this. Checking the CVS logs, it seems that this code has been there at least since we moved Catalina over to the new "jakarta-tomcat-4.0" CVS repository in August. Remy
Re: [TC4] ResourcesBase.setCheckInterval() bug and HTTP
Remy Maucherat wrote:
You're correct that this kind of code is appropriate (because the
component
has already been started without the thread).
Really ? The threadStart() call is in the start() method, and
threadStop()
is always called in stop(). How would the thread need to be started if
the
component is not started yet ?
Consider that you might initialize a resources object with this (among
other
stuff):
resources.setCheckInterval(0);
resources.start();
and later on, while the container is running, an admin application
executes
this:
resources.setCheckInterval(15);
The thread wasn't started inside resources.start() because the check
interval
was zero at that time. Therefore, it *does* need to be started if
setCheckInterval() is called later.
if (started) {
if ((oldCheckInterval 0) (this.checkInterval = 0))
threadStop();
else if ((oldCheckInterval = 0) (this.checkInterval 0))
threadStart();
}
I think it is.
In your case, the first setCheckInterval(0) won't start the thread, as
started = false.
The started flag is switched to true by start().
The setCheckInterval(15) will start the thread correctly, since (started) is
true, and ((oldCheckInterval = 0) (this.checkInterval 0)) is true too
(the old value was too low, the new one is right).
For long-running servers, we need to remember that configuration is not a
one-time event. (By the way, IMHO, this is one of the few places where
Avalon
doesn't quite "get it".)
There was a reconfigurable interface at some point. I don't know if it's
still there.
The new HTTP stuff looks like it's working. I tested with all my servlets
suite (including Slide), and I was using ridiculously small buffers (between
1 and 4 bytes) while testing.
HttpProcessor is still very similar to what it was before (but not for
long). Basically, what I did is I wrote a set of buffer classes (which I can
recycle), and I wraped the socket input stream using a tweaked
BufferedInputStream. This input stream directly manipulates the internal
buffer and the position pointer to achieve maximum performance. I don't
think that part can be optimized more, since whatever happens, I'm only
reading a character (from the internal buffer) once for every operation
(reading the request line or reading the headers).
What is coming next is that the HttpProcessor and the Request object will
only use those recyclable objects (instead of Strings). Hopefully, the only
things which will have to be garbage collected is the Strings generated for
many of the servlet API function calls. That should bring another very nice
increase in performance :)
After that, I'll optimize the output, as Costin suggested.
Remy
