Bug 62168

Summary: PersistentManager.minIdleSwap=-1 does not disable swapping as documented.
Product: Tomcat 9 Reporter: Holger Sunke <holger.sunke>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Severity: normal    
Priority: P2    
Version: 9.0.5   
Target Milestone: -----   
Hardware: PC   
OS: Windows NT   
Attachments: Patch against Apache Tomcat trunk r1826332

Description Holger Sunke 2018-03-09 12:03:25 UTC
Created attachment 35758 [details]
Patch against Apache Tomcat trunk r1826332


Documentation says:

The minimum time in seconds a session must be idle before it is eligible to be swapped to disk to keep the active session count below maxActiveSessions. Setting to -1 means sessions will not be swapped out to keep the active session count down. If specified, this value should be less than that specified by maxIdleSwap. By default, this value is set to -1.

The sentence "Setting to -1 means sessions will not be swapped out to keep the active session count down." is not true: sessions are swapped when maxActiveSessions is reached even if minIdleSwap is -1.

The Method
 * Swap idle sessions out to Store if too many are active

does not consider the minIdleSwap < 0 case, but simply swaps out sessions if (timeIdle >= minIdleSwap) which behaves just like setting minIdleSwap=0.

This way, the manager likely swaps sessions that are still quite active at that moment in contrast of the administrators' intention who wantet to disable this feature.

I suggest attached patch to fix this.

Kind regards
Comment 1 Mark Thomas 2018-03-09 20:00:58 UTC
Thanks for the report and the patch.

Fixed in:
- trunk for 9.0.7 onwards
- 8.5.x for 8.5.30 onwards
- 8.0.x for 8.0.51 onwards
- 7.0.x for 7.0.86 onwards
Comment 2 Felix Schumacher 2018-03-10 12:35:19 UTC
The old behaviour would guard against too many sessions in memory. The new one could lead to memory overload.

Maybe we should add a warning, if max sessions limit was reached?
Comment 3 Mark Thomas 2018-03-10 12:44:42 UTC
That is not correct. Look at how maxActiveSessions is used, particularly in createSession.
Comment 4 Felix Schumacher 2018-03-10 13:15:11 UTC
Ok, I spoke without looking at the code. I believe you are referring to 

       if ((maxActiveSessions >= 0) &&
                (getActiveSessions() >= maxActiveSessions)) {
            throw new TooManyActiveSessionsException(

in ManagerBase, correct?

But I still believe the old behaviour was not completely wrong, either.
Comment 5 Mark Thomas 2018-03-12 10:46:32 UTC
The old behaviour was wrong because it did not differentiate between minIdleSwap==0 and minIdleSwap==-1