Bug 43228 - StandardWrapper#countAllocated isn't thread-safe
Summary: StandardWrapper#countAllocated isn't thread-safe
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 6.0.14
Hardware: Other other
: P2 normal (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2007-08-28 05:34 UTC by Takayuki Kaneko
Modified: 2009-02-04 17:08 UTC (History)
1 user (show)

patch against tc6.0.x (3.16 KB, patch)
2007-08-28 05:35 UTC, Takayuki Kaneko
Details | Diff
benchmark results (2.46 KB, application/zip)
2007-08-29 05:23 UTC, Takayuki Kaneko
patch for document if needed (653 bytes, patch)
2007-08-29 05:25 UTC, Takayuki Kaneko
Details | Diff
benchmark results with a simpler servlet (5.71 KB, application/zip)
2007-08-29 22:09 UTC, Takayuki Kaneko

Note You need to log in before you can comment on or make changes to this bug.
Description Takayuki Kaneko 2007-08-28 05:34:14 UTC

I noticed that StandardWrapper#countAllocated isn't thread-safe.
So, StandardWrapper#unload cannot finish until nRetries will be 21(40 seconds)
because countAllocated won't be 0.

I made a patch.

Comment 1 Takayuki Kaneko 2007-08-28 05:35:31 UTC
Created attachment 20720 [details]
patch against tc6.0.x
Comment 2 Takayuki Kaneko 2007-08-28 05:48:09 UTC
> (40 seconds)

I'm sorry. It takes 2 seconds per Wrapper.
If Tomcat has 100-Wrappers and their countAllocated are incorrect, it takes 200
seconds to shutdown them.

Comment 3 Remy Maucherat 2007-08-28 06:03:42 UTC
As usual, I am not in favor of applying this sort of patch, given the possible
high synchronization vs the actual severity of the issue. Feel free to continue
using your patch if it works for you.
Comment 4 Takayuki Kaneko 2007-08-29 05:22:34 UTC
Hi Remy,
thanks for response.

I have respects for your carefulness about the performance.

IMO, AtomicInteger doesn't have synchronization problem and it's overhead is
really low.
I did performance benchmarks using Apache Bench between original and my patch .

Here are the summary of results.

Requests per second:
original  5743.66 [#/sec] (mean)
patch     5748.83 [#/sec] (mean)

99% Percentage of the requests served within a certain time (ms)
original  96
patch     80

Average CPU Usage
original 96.35%
patch    96.25%

Is this reasonable? How do you think? 

BTW, the actual severity is that we cannot shutdown Tomcat quickly.
We can avoid this problem to set the StandardContext#unloadDelay as 0.
If this issue is WONTFIX, please note it on document instead.
Comment 5 Takayuki Kaneko 2007-08-29 05:23:30 UTC
Created attachment 20725 [details]
benchmark results
Comment 6 Takayuki Kaneko 2007-08-29 05:25:52 UTC
Created attachment 20726 [details]
patch for document if needed
Comment 7 Remy Maucherat 2007-08-29 07:15:07 UTC
The HelloWorldExample has a significant overhead, unfortunately. As I said
previously, I don't think this patch is needed, sorry, especially since few
people see this as an issue. Please do not reopen the report, thanks.
Comment 8 Takayuki Kaneko 2007-08-29 22:09:59 UTC
Created attachment 20738 [details]
benchmark results with a simpler servlet

Hi Remy,

Did you mean that a significant overhead of HelloExampleServlet is
ResourceBundle.getBundle() or concatenating strings?

Anyway I did tests with a simpler Servlet.

testname, rps, 99% tile, cpu
original, 6777.25 [#/sec], 75 [ms], 86.90 [%]
patch,	  6873.97 [#/sec], 60 [ms], 86.29 [%]

And I did same tests with Keep-Alive.

testname, rps, 99% tile, cpu 
original, 10137.90 [#/sec], 60 [ms], 97.32 [%]
patch,	  10130.51 [#/sec], 63 [ms], 96.46 [%]

As a result, I could not see the overhead of using AtomicInteger on Tomcat.

However, this issue is not so serious problem because countAllocated is not
used on other places and we have already gotten a workalound, so I consent this
issue is WONTFIX.

I will never reopen it. :-) thanks a lot.
Comment 9 Remy Maucherat 2007-08-30 16:17:39 UTC
Yes, HelloWorldExample does a number of expensive operations.

OTOH, the shutdown strategy is not optimal at all, and has a lot of ways to go
wrong. It should use the time where the connectors (or the webapp) were
shutdown, rather that an individual per servlet time.
Comment 10 Takayuki Kaneko 2008-08-12 09:32:44 UTC
*** Bug 45608 has been marked as a duplicate of this bug. ***
Comment 11 Yao Qi 2009-02-04 17:08:44 UTC
I am the reporter of Bug 45608.  We use MTRAT(http://www.alphaworks.ibm.com/tech/mtrat) to find race conditions in Java program, and Bug 45608 is found in this way.