Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
From: bugzilla@apache.org
To: dev@tomcat.apache.org
Subject: DO NOT REPLY [Bug 53022] session is expired/removed/not found by
 unknown reason
Date: Mon, 02 Apr 2012 21:25:31 +0000
Message-ID: <bug-53022-78-XJFDuqv3HY@https.issues.apache.org/bugzilla/>
In-Reply-To: <bug-53022-78@https.issues.apache.org/bugzilla/>
References: <bug-53022-78@https.issues.apache.org/bugzilla/>
Auto-Submitted: auto-generated
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0

https://issues.apache.org/bugzilla/show_bug.cgi?id=53022

Mark Thomas <markt@apache.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|                            |INVALID

--- Comment #5 from Mark Thomas <markt@apache.org> 2012-04-02 21:25:31 UTC ---
(In reply to comment #4)
> Ok, I'll explain it in deep. Look this code in
> org.apache.catalina.session.ManagerBase
> 
>     protected String generateSessionId() {
> 
>         String result = null;
> 
>         do {
>             if (result != null) {
>                 // Not thread-safe but if one of multiple increments is lost
>                 // that is not a big deal since the fact that there was any
>                 // duplicate is a much bigger issue.
>                 duplicates++;
>             }
> 
>             result = sessionIdGenerator.generateSessionId();
> 
>         } while (sessions.containsKey(result));
> 
>         return result;
>     }
> 
> But the in tomcat 7.0.23 was
> 
>     protected synchronized String generateSessionId() {
> ......

No it wasn't. The code quoted for 7.0.23 is incorrect. The only code changes to
ManagerBase since 7.0.23 are to add some comments and to make some fields
final.


> The code was changed in 7.0.26 from use just one SecureRandom to:
> 
> private Queue<SecureRandom> randoms = new
> ConcurrentLinkedQueue<SecureRandom>();

Wrong again. That change was introduced in 7.0.5.


> There is one good reason for use a synchronized method in that part. Before
> Java 7, there is no mention anywere on java spec that SecureRandom is thread
> safe. To ensure thread safety, SecureRandom and the algorithm should ensure
> thread safety (look than in 7.0.26, the algorihm can be configured, are you
> 100% sure the algorithm selected will be thread safe?). Being strict with java
> spec, you should use a synchronized block in that part.

Wrong again. There is no concurrent access to SecureRandom instances.


> But the worst part is the use of a queue of SecureRandom instances and the way
> the algorithm check for duplicates. Since the containsKey() operation and the
> further put() operation occur in different places (see ManagerBase.add()),
> there is no warrant a duplicate sessionid cannot be included in the time
> between containsKey() and put() and since you have multiple SecureRandom
> instances this risk is even more evident.

I agree the duplicate check is incomplete but I believe it to be completely
unnecessary. The odds of getting a duplicate are so astronomically small that I
do not believe we should even bother checking. That code is a left over from
very early days of Tomcat when insecure random number generators were used that
did have a realistic chance of generating a duplicate.


> Even if the probability that those events could happen is very, very, very
> small, it is clear that ManagerBase.add() operation should do the check for
> duplicates too and if that so, try to generate and alternate identifier.

As I stated above, I disagree. The check is pointless.


> Note how tricky is all this. Reproduce this failure is something almost
> impossible, one in a million. There is no unit test that could catch it, but
> the bug exists, it is theorically feasible.

The odds are an awful lot longer than that.


> After investigating my particular problem, I found that my problem was more
> related with JMeter. In my load test I used the default Java for HTTP protocol
> implementation (because is a little bit faster) but since there is no control
> over connection reuse, Tomcat throw SocketException after some time without log
> it in the default logger. Switching to HttpClient4 solves the problem.

Which just confirms my point that this was not a Tomcat bug at all. Depending
on what triggered the exception, it may well be logged below INFO level and
therefore not be logged by default.


> It is curious how many users has been caught over the time into the same
> problem (JMeter-Tomcat). After all, the solution is not trivial. 
> 
> Please be more kind in the future. By experience I know that sometimes gather
> evidence about these kind of bugs is very difficult, so don't throw away such
> reports, without consider them first.

The 'kindness' of a response is largely proportional to the quality of the bug
report. Lets take a look at your initial report:
- Tomcat version provided? Yes.
- Test case to reproduce provided? No.
- Description of how to reproduce? No.
- Configuration details for Tomcat (Connector, changes to defaults, etc.)
provided? No.
- Configuration for JMeter provided? No.
- Chances of anyone being able to recreate the problem you were seeing? Zero.
- Chances of anyone being able to make an educated guess as to the cause of the
problem you were seeing? Zero.
- Evidence of further research on your part to reduce search space (e.g.
testing with 7.0.25) ? None.

Overall, a fairly useless bug report. Given the quality of the bug report, my
response was pretty mild.

Moving on to comment #4 ...

Your comments on the correctness of the duplicate check are valid but the rest
is way off the mark. There are multiple incorrect statements about when changes
were introduced followed by some incorrect concurrency analysis. Overall I'd
rate it as marginally useful since it has reminded me to look at removing the
pointless duplicate check.

If you choose to respond to this, then I strongly recommend that you read this
first:
http://www.catb.org/~esr/faqs/smart-questions.html

and respond on the users list. Bugzilla is not a discussion forum.

-- 
Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org