Bug 62168 - PersistentManager.minIdleSwap=-1 does not disable swapping as documented.
Summary: PersistentManager.minIdleSwap=-1 does not disable swapping as documented.
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 9.0.5
Hardware: PC Windows NT
: P2 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-09 12:03 UTC by Holger Sunke
Modified: 2018-03-12 10:46 UTC (History)
0 users



Attachments
Patch against Apache Tomcat trunk r1826332 (607 bytes, patch)
2018-03-09 12:03 UTC, Holger Sunke
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Sunke 2018-03-09 12:03:25 UTC
Created attachment 35758 [details]
Patch against Apache Tomcat trunk r1826332

Hello,

Documentation says:

minIdleSwap	
-----------
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
*/
org.apache.catalina.session.PersistentManagerBase.processMaxActiveSwaps()

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)) {
            rejectedSessions++;
            throw new TooManyActiveSessionsException(
                    sm.getString("managerBase.createSession.ise"),
                    maxActiveSessions);
        }

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