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 2010/11/18 20:59:11 UTC
svn commit: r1036595 - in /tomcat/trunk: java/org/apache/catalina/
java/org/apache/catalina/ha/session/ java/org/apache/catalina/session/
test/org/apache/catalina/session/
Author: markt
Date: Thu Nov 18 19:59:11 2010
New Revision: 1036595
URL: http://svn.apache.org/viewvc?rev=1036595&view=rev
Log:
Fix expiration statistics broken by r1036281
Add session creation and expiration rate statistics based on the 100 most recently created/expired sessions
Modify average session alive time to also use 100 most recently expired sessions
Update benchmarks - new statistics add overhead but not significant in overall processing chain
Modified:
tomcat/trunk/java/org/apache/catalina/Manager.java
tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java
tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
tomcat/trunk/java/org/apache/catalina/session/PersistentManagerBase.java
tomcat/trunk/java/org/apache/catalina/session/StandardSession.java
tomcat/trunk/java/org/apache/catalina/session/mbeans-descriptors.xml
tomcat/trunk/test/org/apache/catalina/session/Benchmarks.java
Modified: tomcat/trunk/java/org/apache/catalina/Manager.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/Manager.java?rev=1036595&r1=1036594&r2=1036595&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/Manager.java (original)
+++ tomcat/trunk/java/org/apache/catalina/Manager.java Thu Nov 18 19:59:11 2010
@@ -213,24 +213,30 @@ public interface Manager {
/**
* Gets the average time (in seconds) that expired sessions had been
- * alive.
- *
+ * alive. This may be based on sample data.
+ *
* @return Average time (in seconds) that expired sessions had been
* alive.
*/
public int getSessionAverageAliveTime();
-
+
/**
- * Sets the average time (in seconds) that expired sessions had been
- * alive.
- *
- * @param sessionAverageAliveTime Average time (in seconds) that expired
- * sessions had been alive.
+ * Gets the current rate of session creation (in session per minute). This
+ * may be based on sample data.
+ *
+ * @return The current rate (in sessions per minute) of session creation
*/
- public void setSessionAverageAliveTime(int sessionAverageAliveTime);
-
+ public int getSessionCreateRate();
+
+ /**
+ * Gets the current rate of session expiration (in session per minute). This
+ * may be based on sample data
+ *
+ * @return The current rate (in sessions per minute) of session expiration
+ */
+ public int getSessionExpireRate();
// --------------------------------------------------------- Public Methods
@@ -326,6 +332,15 @@ public interface Manager {
/**
+ * Remove this Session from the active Sessions for this Manager.
+ *
+ * @param session Session to be removed
+ * @param update Should the expiration statistics be updated
+ */
+ public void remove(Session session, boolean update);
+
+
+ /**
* Remove a property change listener from this component.
*
* @param listener The listener to remove
Modified: tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java?rev=1036595&r1=1036594&r2=1036595&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java (original)
+++ tomcat/trunk/java/org/apache/catalina/ha/session/DeltaManager.java Thu Nov 18 19:59:11 2010
@@ -40,6 +40,7 @@ import org.apache.catalina.ha.CatalinaCl
import org.apache.catalina.ha.ClusterManager;
import org.apache.catalina.ha.ClusterMessage;
import org.apache.catalina.ha.tcp.ReplicationValve;
+import org.apache.catalina.session.ManagerBase;
import org.apache.catalina.tribes.Member;
import org.apache.catalina.tribes.io.ReplicationStream;
import org.apache.tomcat.util.ExceptionUtils;
@@ -1117,7 +1118,17 @@ public CatalinaCluster getCluster() {
*/
public synchronized void resetStatistics() {
processingTime = 0 ;
- expiredSessions = 0 ;
+ expiredSessions.set(0);
+ sessionCreationTiming.clear();
+ while (sessionCreationTiming.size() <
+ ManagerBase.TIMING_STATS_CACHE_SIZE) {
+ sessionCreationTiming.add(null);
+ }
+ sessionExpirationTiming.clear();
+ while (sessionExpirationTiming.size() <
+ ManagerBase.TIMING_STATS_CACHE_SIZE) {
+ sessionExpirationTiming.add(null);
+ }
rejectedSessions = 0 ;
sessionReplaceCounter = 0 ;
counterNoStateTransfered = 0 ;
Modified: tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java?rev=1036595&r1=1036594&r2=1036595&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java (original)
+++ tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java Thu Nov 18 19:59:11 2010
@@ -34,15 +34,20 @@ import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.PrivilegedAction;
import java.security.SecureRandom;
+import java.util.ArrayList;
import java.util.Date;
+import java.util.Deque;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Iterator;
+import java.util.LinkedList;
+import java.util.List;
import java.util.Map;
import java.util.Queue;
import java.util.Random;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.atomic.AtomicLong;
import org.apache.catalina.Container;
import org.apache.catalina.Context;
@@ -183,16 +188,18 @@ public abstract class ManagerBase extend
private final Object sessionMaxAliveTimeUpdateLock = new Object();
- /**
- * Average time (in seconds) that expired sessions had been alive.
- */
- protected int sessionAverageAliveTime;
+ protected static final int TIMING_STATS_CACHE_SIZE = 100;
+ protected Deque<SessionTiming> sessionCreationTiming =
+ new LinkedList<SessionTiming>();
+
+ protected Deque<SessionTiming> sessionExpirationTiming =
+ new LinkedList<SessionTiming>();
/**
* Number of sessions that have expired.
*/
- protected long expiredSessions = 0;
+ protected AtomicLong expiredSessions = new AtomicLong(0);
/**
@@ -760,7 +767,7 @@ public abstract class ManagerBase extend
*/
@Override
public long getExpiredSessions() {
- return expiredSessions;
+ return expiredSessions.get();
}
@@ -771,7 +778,7 @@ public abstract class ManagerBase extend
*/
@Override
public void setExpiredSessions(long expiredSessions) {
- this.expiredSessions = expiredSessions;
+ this.expiredSessions.set(expiredSessions);
}
public long getProcessingTime() {
@@ -863,6 +870,15 @@ public abstract class ManagerBase extend
randomInputStreams.add(is);
}
+ // Ensure caches for timing stats are the right size by filling with
+ // nulls.
+ while (sessionCreationTiming.size() < TIMING_STATS_CACHE_SIZE) {
+ sessionCreationTiming.add(null);
+ }
+ while (sessionExpirationTiming.size() < TIMING_STATS_CACHE_SIZE) {
+ sessionExpirationTiming.add(null);
+ }
+
// Force initialization of the random number generator
if (log.isDebugEnabled())
log.debug("Force random number initialization starting");
@@ -948,6 +964,11 @@ public abstract class ManagerBase extend
session.setId(id);
sessionCounter++;
+ SessionTiming timing = new SessionTiming(session.getCreationTime(), 0);
+ synchronized (sessionCreationTiming) {
+ sessionCreationTiming.add(timing);
+ sessionCreationTiming.poll();
+ }
return (session);
}
@@ -1004,20 +1025,29 @@ public abstract class ManagerBase extend
*/
@Override
public void remove(Session session) {
+ remove(session, false);
+ }
+
+ /**
+ * Remove this Session from the active Sessions for this Manager.
+ *
+ * @param session Session to be removed
+ * @param update Should the expiration statistics be updated
+ */
+ @Override
+ public void remove(Session session, boolean update) {
// If the session has expired - as opposed to just being removed from
// the manager because it is being persisted - update the expired stats
- if (!session.isValid()) {
+ if (update) {
long timeNow = System.currentTimeMillis();
int timeAlive = (int) ((timeNow - session.getCreationTime())/1000);
updateSessionMaxAliveTime(timeAlive);
- synchronized (this) {
- long numExpired = getExpiredSessions();
- numExpired++;
- setExpiredSessions(numExpired);
- int average = getSessionAverageAliveTime();
- average = (int) (((average * (numExpired-1)) + timeAlive)/numExpired);
- setSessionAverageAliveTime(average);
+ expiredSessions.incrementAndGet();
+ SessionTiming timing = new SessionTiming(timeNow, timeAlive);
+ synchronized (sessionExpirationTiming) {
+ sessionExpirationTiming.add(timing);
+ sessionExpirationTiming.poll();
}
}
@@ -1322,27 +1352,124 @@ public abstract class ManagerBase extend
/**
* Gets the average time (in seconds) that expired sessions had been
- * alive.
- *
+ * alive based on the last 100 sessions to expire. If less than
+ * 100 sessions have expired then all available data is used.
+ *
* @return Average time (in seconds) that expired sessions had been
* alive.
*/
@Override
public int getSessionAverageAliveTime() {
- return sessionAverageAliveTime;
+ // Copy current stats
+ List<SessionTiming> copy = new ArrayList<SessionTiming>();
+ synchronized (sessionExpirationTiming) {
+ copy.addAll(sessionExpirationTiming);
+ }
+
+ // Init
+ int counter = 0;
+ int result = 0;
+ Iterator<SessionTiming> iter = copy.iterator();
+
+ // Calculate average
+ while (iter.hasNext()) {
+ SessionTiming timing = iter.next();
+ if (timing != null) {
+ int timeAlive = timing.getDuration();
+ counter++;
+ // Very careful not to overflow - probably not necessary
+ result =
+ (result * ((counter - 1)/counter)) + (timeAlive/counter);
+ }
+ }
+ return result;
}
+
+ /**
+ * Gets the current rate of session creation (in session per minute) based
+ * on the creation time of the previous 100 sessions created. If less than
+ * 100 sessions have been created then all available data is used.
+ *
+ * @return The current rate (in sessions per minute) of session creation
+ */
+ @Override
+ public int getSessionCreateRate() {
+ long now = System.currentTimeMillis();
+ // Copy current stats
+ List<SessionTiming> copy = new ArrayList<SessionTiming>();
+ synchronized (sessionCreationTiming) {
+ copy.addAll(sessionCreationTiming);
+ }
+
+ // Init
+ long oldest = now;
+ int counter = 0;
+ int result = 0;
+ Iterator<SessionTiming> iter = copy.iterator();
+
+ // Calculate rate
+ while (iter.hasNext()) {
+ SessionTiming timing = iter.next();
+ if (timing != null) {
+ counter++;
+ if (timing.getTimestamp() < oldest) {
+ oldest = timing.getTimestamp();
+ }
+ }
+ }
+ if (counter > 0) {
+ if (oldest < now) {
+ result = (int) ((1000*60*counter)/(now - oldest));
+ } else {
+ result = Integer.MAX_VALUE;
+ }
+ }
+ return result;
+ }
+
/**
- * Sets the average time (in seconds) that expired sessions had been
- * alive.
- *
- * @param sessionAverageAliveTime Average time (in seconds) that expired
- * sessions had been alive.
+ * Gets the current rate of session expiration (in session per minute) based
+ * on the expiry time of the previous 100 sessions expired. If less than
+ * 100 sessions have expired then all available data is used.
+ *
+ * @return The current rate (in sessions per minute) of session expiration
*/
@Override
- public void setSessionAverageAliveTime(int sessionAverageAliveTime) {
- this.sessionAverageAliveTime = sessionAverageAliveTime;
+ public int getSessionExpireRate() {
+ long now = System.currentTimeMillis();
+ // Copy current stats
+ List<SessionTiming> copy = new ArrayList<SessionTiming>();
+ synchronized (sessionExpirationTiming) {
+ copy.addAll(sessionExpirationTiming);
+ }
+
+ // Init
+ long oldest = now;
+ int counter = 0;
+ int result = 0;
+ Iterator<SessionTiming> iter = copy.iterator();
+
+ // Calculate rate
+ while (iter.hasNext()) {
+ SessionTiming timing = iter.next();
+ if (timing != null) {
+ counter++;
+ if (timing.getTimestamp() < oldest) {
+ oldest = timing.getTimestamp();
+ }
+ }
+ }
+ if (counter > 0) {
+ if (oldest < now) {
+ result = (int) ((1000*60*counter)/(now - oldest));
+ } else {
+ // Better than reporting zero
+ result = Integer.MAX_VALUE;
+ }
+ }
+ return result;
}
@@ -1552,4 +1679,31 @@ public abstract class ManagerBase extend
}
}
}
+
+ // ----------------------------------------------------------- Inner classes
+
+ protected static final class SessionTiming {
+ private long timestamp;
+ private int duration;
+
+ public SessionTiming(long timestamp, int duration) {
+ this.timestamp = timestamp;
+ this.duration = duration;
+ }
+
+ /**
+ * Time stamp associated with this piece of timing information in
+ * milliseconds.
+ */
+ public long getTimestamp() {
+ return timestamp;
+ }
+
+ /**
+ * Duration associated with this piece of timing information in seconds.
+ */
+ public int getDuration() {
+ return duration;
+ }
+ }
}
Modified: tomcat/trunk/java/org/apache/catalina/session/PersistentManagerBase.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/session/PersistentManagerBase.java?rev=1036595&r1=1036594&r2=1036595&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/session/PersistentManagerBase.java (original)
+++ tomcat/trunk/java/org/apache/catalina/session/PersistentManagerBase.java Thu Nov 18 19:59:11 2010
@@ -438,7 +438,7 @@ public abstract class PersistentManagerB
log.debug("Start expire sessions " + getName() + " at " + timeNow + " sessioncount " + sessions.length);
for (int i = 0; i < sessions.length; i++) {
if (!sessions[i].isValid()) {
- expiredSessions++;
+ expiredSessions.incrementAndGet();
expireHere++;
}
}
Modified: tomcat/trunk/java/org/apache/catalina/session/StandardSession.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/session/StandardSession.java?rev=1036595&r1=1036594&r2=1036595&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/session/StandardSession.java (original)
+++ tomcat/trunk/java/org/apache/catalina/session/StandardSession.java Thu Nov 18 19:59:11 2010
@@ -836,7 +836,7 @@ public class StandardSession
setValid(false);
// Remove this session from our manager's active sessions
- manager.remove(this);
+ manager.remove(this, true);
// Notify interested session event listeners
if (notify) {
Modified: tomcat/trunk/java/org/apache/catalina/session/mbeans-descriptors.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/session/mbeans-descriptors.xml?rev=1036595&r1=1036594&r2=1036595&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/catalina/session/mbeans-descriptors.xml (original)
+++ tomcat/trunk/java/org/apache/catalina/session/mbeans-descriptors.xml Thu Nov 18 19:59:11 2010
@@ -96,12 +96,23 @@
<attribute name="sessionAverageAliveTime"
description="Average time an expired session had been alive"
- type="int" />
+ type="int"
+ writeable="false" />
+
+ <attribute name="sessionCreateRate"
+ description="Session creation rate in sessions per minute"
+ type="int"
+ writeable="false" />
<attribute name="sessionCounter"
description="Total number of sessions created by this manager"
type="long" />
+ <attribute name="sessionExpireRate"
+ description="Session expiration rate in sessions per minute"
+ type="int"
+ writeable="false" />
+
<attribute name="sessionIdLength"
description="The session id length (in bytes) of Sessions
created by this Manager"
@@ -300,12 +311,23 @@
<attribute name="sessionAverageAliveTime"
description="Average time an expired session had been alive"
- type="int" />
+ type="int"
+ writeable="false" />
+
+ <attribute name="sessionCreateRate"
+ description="Session creation rate in sessions per minute"
+ type="int"
+ writeable="false" />
<attribute name="sessionCounter"
description="Total number of sessions created by this manager"
type="long" />
+ <attribute name="sessionExpireRate"
+ description="Session expiration rate in sessions per minute"
+ type="int"
+ writeable="false" />
+
<attribute name="sessionIdLength"
description="The session id length (in bytes) of Sessions
created by this Manager"
Modified: tomcat/trunk/test/org/apache/catalina/session/Benchmarks.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/session/Benchmarks.java?rev=1036595&r1=1036594&r2=1036595&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/session/Benchmarks.java (original)
+++ tomcat/trunk/test/org/apache/catalina/session/Benchmarks.java Thu Nov 18 19:59:11 2010
@@ -31,10 +31,10 @@ public class Benchmarks extends TestCase
/*
* Results on markt's 4-core Windows dev box
- * 1 thread - ~1,900ms
+ * 1 thread - ~2,000ms
* 2 threads - ~3,300ms
- * 4 threads - ~4,800ms
- * 16 threads - ~21,000ms
+ * 4 threads - ~4,900ms
+ * 16 threads - ~21,300ms
*
* Results on markt's 2-core OSX dev box
* 1 thread - ~5,600ms
@@ -68,6 +68,14 @@ public class Benchmarks extends TestCase
mgr.randomFileCurrent = mgr.randomFile;
mgr.createRandomInputStream();
mgr.generateSessionId();
+ while (mgr.sessionCreationTiming.size() <
+ ManagerBase.TIMING_STATS_CACHE_SIZE) {
+ mgr.sessionCreationTiming.add(null);
+ }
+ while (mgr.sessionExpirationTiming.size() <
+ ManagerBase.TIMING_STATS_CACHE_SIZE) {
+ mgr.sessionExpirationTiming.add(null);
+ }
Thread[] threads = new Thread[threadCount];
@@ -128,10 +136,10 @@ public class Benchmarks extends TestCase
/*
* Results on markt's 4-core Windows dev box
- * 1 thread - ~4,600ms
- * 2 threads - ~6,700ms
- * 4 threads - ~10,400ms
- * 16 threads - ~43,800ms
+ * 1 thread - ~4,300ms
+ * 2 threads - ~7,600ms
+ * 4 threads - ~11,600ms
+ * 16 threads - ~49,000ms
*
* Results on markt's 2-core OSX dev box
* 1 thread - ~9,100ms
@@ -159,6 +167,14 @@ public class Benchmarks extends TestCase
mgr.randomFileCurrent = mgr.randomFile;
mgr.createRandomInputStream();
mgr.generateSessionId();
+ while (mgr.sessionCreationTiming.size() <
+ ManagerBase.TIMING_STATS_CACHE_SIZE) {
+ mgr.sessionCreationTiming.add(null);
+ }
+ while (mgr.sessionExpirationTiming.size() <
+ ManagerBase.TIMING_STATS_CACHE_SIZE) {
+ mgr.sessionExpirationTiming.add(null);
+ }
Thread[] threads = new Thread[threadCount];
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1036595 - in /tomcat/trunk: java/org/apache/catalina/
java/org/apache/catalina/ha/session/ java/org/apache/catalina/session/ test/org/apache/catalina/session/
Posted by Mladen Turk <mt...@apache.org>.
On 11/25/2010 05:33 PM, Mark Thomas wrote:
>
> How about this as an approach to reduce the complexity:
> 1. Remove the MD5 code (optional)
> 2. Default to /dev/urandom then SecureRandom. Don't fall back to Random.
> 3. Provide a class that implements Random that reads data from a file
> 4. If randomFile is specified, use the the class from 3 as the Random source
>
> That should reduce the current 3 Queues to one. I doubt it will improve
> performance much but it should make the code clearer.
>
> Thoughts?
>
How about using OS.random if APR is present?
It will use APR's apr_generate_random_bytes which
will OTOH use OS optimal random bytes generation
eg, dev random, EGD-socket, CryptGenRandom, ...
Regards
--
^TM
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1036595 - in /tomcat/trunk: java/org/apache/catalina/
java/org/apache/catalina/ha/session/ java/org/apache/catalina/session/ test/org/apache/catalina/session/
Posted by Mark Thomas <ma...@apache.org>.
On 29/11/2010 15:52, Konstantin Kolinko wrote:
> 2010/11/29 Mark Thomas <ma...@apache.org>:
>> Good to see we were thinking along the same lines. I still want to get
>> to the bottom of the really poor performance on my Mac.
>
> Looking at documentation for SecureRandom() constructor, it uses
> whatever implementation that it finds first. So, configuration of JRE
> might be important.
I'm leaning towards making provider and algorithm configurable and
defaulting to one that is reasonably quick (i.e. whatever Windows is
using which I think is a SHA1 based PRNG).
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1036595 - in /tomcat/trunk: java/org/apache/catalina/
java/org/apache/catalina/ha/session/ java/org/apache/catalina/session/ test/org/apache/catalina/session/
Posted by Konstantin Kolinko <kn...@gmail.com>.
2010/11/29 Mark Thomas <ma...@apache.org>:
> Good to see we were thinking along the same lines. I still want to get
> to the bottom of the really poor performance on my Mac.
Looking at documentation for SecureRandom() constructor, it uses
whatever implementation that it finds first. So, configuration of JRE
might be important.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1036595 - in /tomcat/trunk: java/org/apache/catalina/
java/org/apache/catalina/ha/session/ java/org/apache/catalina/session/ test/org/apache/catalina/session/
Posted by Mark Thomas <ma...@apache.org>.
On 29/11/2010 13:41, Tim Funk wrote:
> Sorry for the additional noise ... my svn emails are in a different
> folder from dev emails. I just noticed ...
Good to see we were thinking along the same lines. I still want to get
to the bottom of the really poor performance on my Mac. Before I do
that, I want to take a look at the recent 7.0.5 bugs to see if any are
serious enough to change my vote from Beta.
Mark
>
> svn commit: r1039882 -
> /tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
>
> -Tim
>
> On 11/29/2010 7:40 AM, Tim Funk wrote:
>> I checked the svn history of why MD5 (hashing was used) and the picture
>> is incomplete. (unless someone asks craig since I think he was the
>> author)
>>
>> But it appears like this ...
>> Tomcat 3.X use Math.random() and some misc crap to generate its session
>> id. It had a comment (paraphrased), "not secure for banking/military use
>> for creating session ID. "
>>
>> Then in tomcat 4.0 - we see the code as it is now. It has always tried
>> to use SecureRandom. What is interesting is SecureRandom's javadocs in
>> 1.2 (JDK) are different than 1.4. (And maybe 1.3) By the time we get to
>> Java 1.4 - SecureRandom is advertised as cryptographically strong. So
>> back in the day of 1.2 - there was no guarantee there. Or maybe is
>> wasn't cross platform guaranteed.
>>
>> So back in the day ... you might be able to get a stream of session ids
>> ... and then determine where you are in the sequence of random number
>> generation. (Recall that randoms aren't really random, its just an
>> algorithm on a seed) An "easy" way to thwart this attack - is to hash
>> the numbers. Making it orders of magnitude harder determine what the
>> numbers generated the session ID. (For this type of deception, any
>> hashing function is still OK even with the collision issues)
>>
>> So that leaves us to where we are now. Interestingly enough ... RFC1750
>> (as listed in the SecureRandom javadoc) discourages the use of current
>> time as a seed because the window to guess the seed is now orders of
>> magnitude smaller.
>>
>> Since all instances of Random are self seeding, it may be best (ask you
>> local JVM expert for opinion) to allow the JVM to decide the seed. In
>> which case - it may go to the hardware, use /dev/urandom, etc. Which
>> would be much better than anything we can do.
>>
>> As for hash or not to hash. I am unsure. If the seed IS quality, and the
>> random algorithm is quality - then there is no need to hash. But if we
>> allow reality to intervene - then we might accept that some platforms
>> might not have a quality seed, or one of the algorithms might come under
>> attack and no longer be good. In which case - hashing becomes a good
>> defense.
>>
>>
>> So to collect all the thoughts above ... it might be nice to do the
>> following
>> - Force use of SecureRandom (and still allow it to be extended)
>> - Turn hashing off (but leave it as an option)
>> - When initializing SecureRandom - do nothing. Let the JVM take care
>> of it.
>> - Allow /dev/urandom to override the previous statement.
>>
>> Then if any of the above is unacceptable ... the user can just provide
>> their own extended version of SecureRandom.
>>
>>
>> -Tim
>>
>> On 11/26/2010 1:46 PM, Remy Maucherat wrote:
>>> On Thu, 2010-11-25 at 16:33 +0000, Mark Thomas wrote:
>>>> I wouldn't call it bad. It doesn't do any harm (apart from adding a
>>>> very
>>>> small amount of overhead), and it would help if the random source
>>>> selected ended up not being that random.
>>>>
>>>> I thought the trade-off of protection against bad choices of random
>>>> sources was worth the minimal overhead added. I'm not against removing
>>>> it entirely, if there is consensus to do so.
>>>
>>> MD5 is now known as a bad hash (it was fine at the time the code was
>>> written), so does it actually improve anything ?
>>>
>>> Also, isn't SecureRandom always available now ? This is 10+ years old
>>> code, probably.
>>>
>>>> For SecureRandom, yes. I did test this locally and it achieves
>>>> thread-safety by using an internal sync and it did create a significant
>>>> bottleneck. That is why I went the parallel route. Reading from a
>>>> stream
>>>> has a similar sync so again that is why I went the parallel route.
>>>
>>> Ok. The internal lock is a much smaller sync than the old sync block, so
>>> it would be a bit better. It is possible it is noticeable, though. The
>>> question is if this yields a good enough session creation rate.
>>>
>>>> How about this as an approach to reduce the complexity:
>>>> 1. Remove the MD5 code (optional)
>>>> 2. Default to /dev/urandom then SecureRandom. Don't fall back to
>>>> Random.
>>>> 3. Provide a class that implements Random that reads data from a file
>>>> 4. If randomFile is specified, use the the class from 3 as the Random
>>>> source
>>>>
>>>> That should reduce the current 3 Queues to one. I doubt it will improve
>>>> performance much but it should make the code clearer.
>>>>
>>>> Thoughts?
>>>
>>> I don't know what the best solution is. /dev/urandom could also only be
>>> used as seed only to a SecureRandom, this is more Javaish.
>>> So about the MD5, I don't think it is useful anymore.
>>
>> ---------------------------------------------------------------------
>> 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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1036595 - in /tomcat/trunk: java/org/apache/catalina/
java/org/apache/catalina/ha/session/ java/org/apache/catalina/session/ test/org/apache/catalina/session/
Posted by Tim Funk <fu...@apache.org>.
Sorry for the additional noise ... my svn emails are in a different
folder from dev emails. I just noticed ...
svn commit: r1039882 -
/tomcat/trunk/java/org/apache/catalina/session/ManagerBase.java
-Tim
On 11/29/2010 7:40 AM, Tim Funk wrote:
> I checked the svn history of why MD5 (hashing was used) and the picture
> is incomplete. (unless someone asks craig since I think he was the author)
>
> But it appears like this ...
> Tomcat 3.X use Math.random() and some misc crap to generate its session
> id. It had a comment (paraphrased), "not secure for banking/military use
> for creating session ID. "
>
> Then in tomcat 4.0 - we see the code as it is now. It has always tried
> to use SecureRandom. What is interesting is SecureRandom's javadocs in
> 1.2 (JDK) are different than 1.4. (And maybe 1.3) By the time we get to
> Java 1.4 - SecureRandom is advertised as cryptographically strong. So
> back in the day of 1.2 - there was no guarantee there. Or maybe is
> wasn't cross platform guaranteed.
>
> So back in the day ... you might be able to get a stream of session ids
> ... and then determine where you are in the sequence of random number
> generation. (Recall that randoms aren't really random, its just an
> algorithm on a seed) An "easy" way to thwart this attack - is to hash
> the numbers. Making it orders of magnitude harder determine what the
> numbers generated the session ID. (For this type of deception, any
> hashing function is still OK even with the collision issues)
>
> So that leaves us to where we are now. Interestingly enough ... RFC1750
> (as listed in the SecureRandom javadoc) discourages the use of current
> time as a seed because the window to guess the seed is now orders of
> magnitude smaller.
>
> Since all instances of Random are self seeding, it may be best (ask you
> local JVM expert for opinion) to allow the JVM to decide the seed. In
> which case - it may go to the hardware, use /dev/urandom, etc. Which
> would be much better than anything we can do.
>
> As for hash or not to hash. I am unsure. If the seed IS quality, and the
> random algorithm is quality - then there is no need to hash. But if we
> allow reality to intervene - then we might accept that some platforms
> might not have a quality seed, or one of the algorithms might come under
> attack and no longer be good. In which case - hashing becomes a good
> defense.
>
>
> So to collect all the thoughts above ... it might be nice to do the
> following
> - Force use of SecureRandom (and still allow it to be extended)
> - Turn hashing off (but leave it as an option)
> - When initializing SecureRandom - do nothing. Let the JVM take care of it.
> - Allow /dev/urandom to override the previous statement.
>
> Then if any of the above is unacceptable ... the user can just provide
> their own extended version of SecureRandom.
>
>
> -Tim
>
> On 11/26/2010 1:46 PM, Remy Maucherat wrote:
>> On Thu, 2010-11-25 at 16:33 +0000, Mark Thomas wrote:
>>> I wouldn't call it bad. It doesn't do any harm (apart from adding a very
>>> small amount of overhead), and it would help if the random source
>>> selected ended up not being that random.
>>>
>>> I thought the trade-off of protection against bad choices of random
>>> sources was worth the minimal overhead added. I'm not against removing
>>> it entirely, if there is consensus to do so.
>>
>> MD5 is now known as a bad hash (it was fine at the time the code was
>> written), so does it actually improve anything ?
>>
>> Also, isn't SecureRandom always available now ? This is 10+ years old
>> code, probably.
>>
>>> For SecureRandom, yes. I did test this locally and it achieves
>>> thread-safety by using an internal sync and it did create a significant
>>> bottleneck. That is why I went the parallel route. Reading from a stream
>>> has a similar sync so again that is why I went the parallel route.
>>
>> Ok. The internal lock is a much smaller sync than the old sync block, so
>> it would be a bit better. It is possible it is noticeable, though. The
>> question is if this yields a good enough session creation rate.
>>
>>> How about this as an approach to reduce the complexity:
>>> 1. Remove the MD5 code (optional)
>>> 2. Default to /dev/urandom then SecureRandom. Don't fall back to Random.
>>> 3. Provide a class that implements Random that reads data from a file
>>> 4. If randomFile is specified, use the the class from 3 as the Random
>>> source
>>>
>>> That should reduce the current 3 Queues to one. I doubt it will improve
>>> performance much but it should make the code clearer.
>>>
>>> Thoughts?
>>
>> I don't know what the best solution is. /dev/urandom could also only be
>> used as seed only to a SecureRandom, this is more Javaish.
>> So about the MD5, I don't think it is useful anymore.
>
> ---------------------------------------------------------------------
> 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: r1036595 - in /tomcat/trunk: java/org/apache/catalina/
java/org/apache/catalina/ha/session/ java/org/apache/catalina/session/ test/org/apache/catalina/session/
Posted by Tim Funk <fu...@apache.org>.
I checked the svn history of why MD5 (hashing was used) and the picture
is incomplete. (unless someone asks craig since I think he was the author)
But it appears like this ...
Tomcat 3.X use Math.random() and some misc crap to generate its session
id. It had a comment (paraphrased), "not secure for banking/military use
for creating session ID. "
Then in tomcat 4.0 - we see the code as it is now. It has always tried
to use SecureRandom. What is interesting is SecureRandom's javadocs in
1.2 (JDK) are different than 1.4. (And maybe 1.3) By the time we get to
Java 1.4 - SecureRandom is advertised as cryptographically strong. So
back in the day of 1.2 - there was no guarantee there. Or maybe is
wasn't cross platform guaranteed.
So back in the day ... you might be able to get a stream of session ids
... and then determine where you are in the sequence of random number
generation. (Recall that randoms aren't really random, its just an
algorithm on a seed) An "easy" way to thwart this attack - is to hash
the numbers. Making it orders of magnitude harder determine what the
numbers generated the session ID. (For this type of deception, any
hashing function is still OK even with the collision issues)
So that leaves us to where we are now. Interestingly enough ... RFC1750
(as listed in the SecureRandom javadoc) discourages the use of current
time as a seed because the window to guess the seed is now orders of
magnitude smaller.
Since all instances of Random are self seeding, it may be best (ask you
local JVM expert for opinion) to allow the JVM to decide the seed. In
which case - it may go to the hardware, use /dev/urandom, etc. Which
would be much better than anything we can do.
As for hash or not to hash. I am unsure. If the seed IS quality, and the
random algorithm is quality - then there is no need to hash. But if we
allow reality to intervene - then we might accept that some platforms
might not have a quality seed, or one of the algorithms might come under
attack and no longer be good. In which case - hashing becomes a good
defense.
So to collect all the thoughts above ... it might be nice to do the
following
- Force use of SecureRandom (and still allow it to be extended)
- Turn hashing off (but leave it as an option)
- When initializing SecureRandom - do nothing. Let the JVM take care of it.
- Allow /dev/urandom to override the previous statement.
Then if any of the above is unacceptable ... the user can just provide
their own extended version of SecureRandom.
-Tim
On 11/26/2010 1:46 PM, Remy Maucherat wrote:
> On Thu, 2010-11-25 at 16:33 +0000, Mark Thomas wrote:
>> I wouldn't call it bad. It doesn't do any harm (apart from adding a very
>> small amount of overhead), and it would help if the random source
>> selected ended up not being that random.
>>
>> I thought the trade-off of protection against bad choices of random
>> sources was worth the minimal overhead added. I'm not against removing
>> it entirely, if there is consensus to do so.
>
> MD5 is now known as a bad hash (it was fine at the time the code was
> written), so does it actually improve anything ?
>
> Also, isn't SecureRandom always available now ? This is 10+ years old
> code, probably.
>
>> For SecureRandom, yes. I did test this locally and it achieves
>> thread-safety by using an internal sync and it did create a significant
>> bottleneck. That is why I went the parallel route. Reading from a stream
>> has a similar sync so again that is why I went the parallel route.
>
> Ok. The internal lock is a much smaller sync than the old sync block, so
> it would be a bit better. It is possible it is noticeable, though. The
> question is if this yields a good enough session creation rate.
>
>> How about this as an approach to reduce the complexity:
>> 1. Remove the MD5 code (optional)
>> 2. Default to /dev/urandom then SecureRandom. Don't fall back to Random.
>> 3. Provide a class that implements Random that reads data from a file
>> 4. If randomFile is specified, use the the class from 3 as the Random source
>>
>> That should reduce the current 3 Queues to one. I doubt it will improve
>> performance much but it should make the code clearer.
>>
>> Thoughts?
>
> I don't know what the best solution is. /dev/urandom could also only be
> used as seed only to a SecureRandom, this is more Javaish.
> So about the MD5, I don't think it is useful anymore.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1036595 - in /tomcat/trunk:
java/org/apache/catalina/ java/org/apache/catalina/ha/session/
java/org/apache/catalina/session/ test/org/apache/catalina/session/
Posted by Remy Maucherat <re...@apache.org>.
On Thu, 2010-11-25 at 16:33 +0000, Mark Thomas wrote:
> I wouldn't call it bad. It doesn't do any harm (apart from adding a very
> small amount of overhead), and it would help if the random source
> selected ended up not being that random.
>
> I thought the trade-off of protection against bad choices of random
> sources was worth the minimal overhead added. I'm not against removing
> it entirely, if there is consensus to do so.
MD5 is now known as a bad hash (it was fine at the time the code was
written), so does it actually improve anything ?
Also, isn't SecureRandom always available now ? This is 10+ years old
code, probably.
> For SecureRandom, yes. I did test this locally and it achieves
> thread-safety by using an internal sync and it did create a significant
> bottleneck. That is why I went the parallel route. Reading from a stream
> has a similar sync so again that is why I went the parallel route.
Ok. The internal lock is a much smaller sync than the old sync block, so
it would be a bit better. It is possible it is noticeable, though. The
question is if this yields a good enough session creation rate.
> How about this as an approach to reduce the complexity:
> 1. Remove the MD5 code (optional)
> 2. Default to /dev/urandom then SecureRandom. Don't fall back to Random.
> 3. Provide a class that implements Random that reads data from a file
> 4. If randomFile is specified, use the the class from 3 as the Random source
>
> That should reduce the current 3 Queues to one. I doubt it will improve
> performance much but it should make the code clearer.
>
> Thoughts?
I don't know what the best solution is. /dev/urandom could also only be
used as seed only to a SecureRandom, this is more Javaish.
So about the MD5, I don't think it is useful anymore.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1036595 - in /tomcat/trunk: java/org/apache/catalina/
java/org/apache/catalina/ha/session/ java/org/apache/catalina/session/ test/org/apache/catalina/session/
Posted by Mark Thomas <ma...@apache.org>.
On 25/11/2010 16:10, Remy Maucherat wrote:
> On Thu, 2010-11-18 at 19:59 +0000, markt@apache.org wrote:
>> Author: markt
>> Date: Thu Nov 18 19:59:11 2010
>> New Revision: 1036595
>>
>> URL: http://svn.apache.org/viewvc?rev=1036595&view=rev
>> Log:
>> Fix expiration statistics broken by r1036281
>> Add session creation and expiration rate statistics based on the 100 most recently created/expired sessions
>> Modify average session alive time to also use 100 most recently expired sessions
>> Update benchmarks - new statistics add overhead but not significant in overall processing chain
>
> But going back to the original optimization work, I still don't quite
> understand. As Tim pointed out, the MD5 hash is probably bad, and
I wouldn't call it bad. It doesn't do any harm (apart from adding a very
small amount of overhead), and it would help if the random source
selected ended up not being that random.
I thought the trade-off of protection against bad choices of random
sources was worth the minimal overhead added. I'm not against removing
it entirely, if there is consensus to do so.
> SecureRandom is already internally thread safe. So the simplest
> refactoring (remove the hash and the synchroinized block) was not
> tested. Is it really a big enough bottleneck which would need the more
> complex plumbing to parallelize ?
For SecureRandom, yes. I did test this locally and it achieves
thread-safety by using an internal sync and it did create a significant
bottleneck. That is why I went the parallel route. Reading from a stream
has a similar sync so again that is why I went the parallel route.
How about this as an approach to reduce the complexity:
1. Remove the MD5 code (optional)
2. Default to /dev/urandom then SecureRandom. Don't fall back to Random.
3. Provide a class that implements Random that reads data from a file
4. If randomFile is specified, use the the class from 3 as the Random source
That should reduce the current 3 Queues to one. I doubt it will improve
performance much but it should make the code clearer.
Thoughts?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1036595 - in /tomcat/trunk:
java/org/apache/catalina/ java/org/apache/catalina/ha/session/
java/org/apache/catalina/session/ test/org/apache/catalina/session/
Posted by Remy Maucherat <re...@apache.org>.
On Thu, 2010-11-18 at 19:59 +0000, markt@apache.org wrote:
> Author: markt
> Date: Thu Nov 18 19:59:11 2010
> New Revision: 1036595
>
> URL: http://svn.apache.org/viewvc?rev=1036595&view=rev
> Log:
> Fix expiration statistics broken by r1036281
> Add session creation and expiration rate statistics based on the 100 most recently created/expired sessions
> Modify average session alive time to also use 100 most recently expired sessions
> Update benchmarks - new statistics add overhead but not significant in overall processing chain
But going back to the original optimization work, I still don't quite
understand. As Tim pointed out, the MD5 hash is probably bad, and
SecureRandom is already internally thread safe. So the simplest
refactoring (remove the hash and the synchroinized block) was not
tested. Is it really a big enough bottleneck which would need the more
complex plumbing to parallelize ?
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org