Skip to content

CachingSessionFactory slowness if setPoolSize is called without initializing initial sessionCacheSize #3826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gauravbrills opened this issue Jun 24, 2022 · 2 comments

Comments

@gauravbrills
Copy link

Version :
5.5.10

Migrating from 5.1.13 and 5.5.10

Describe the bug

We were doing a spring 2.6 migratoin and hence also upgrading spring integration version to 5.5.10 . Our existing code used to initialize CachingSessionFactory as below

      CachingSessionFactory<LsEntry> csf = new CachingSessionFactory<LsEntry>(sf ); 
       scsf.setPoolSize(connectionInfo.getPoolSize());
      // Refer https://github.com/spring-projects/spring-integration/pull/2606
      csf.setTestSession(true);
      csf.setSessionWaitTimeout(props.getSftp()
            .getSessionWaitTimeout());

Post migrating we observed that the setPoolSize method was degrading our performance and in itself taking aroudn 30 seconds . This is mainly due to the method calling SimplePool.setPoolSize which as the poolsize was not initialized keeps a default pool size as Integer.MAX_VALUE and hence when setPoolSize tries to reduce the pool size it goes into a long loop in order to clear it up . The same worked in the old version as the old code used to break in case there was nothing returned on poll .

Diff here v5.1.13.RELEASE...v5.5.10#diff-1da249bba02f27de259b1f47687338cb6204ca4754afd457ef82a1d17c912ca1

To Reproduce

The below code

      CachingSessionFactory<LsEntry> csf = new CachingSessionFactory<LsEntry>(sf ); 
       scsf.setPoolSize(connectionInfo.getPoolSize());
      // Refer https://github.com/spring-projects/spring-integration/pull/2606
      csf.setTestSession(true);
      csf.setSessionWaitTimeout(props.getSftp()
            .getSessionWaitTimeout());

Expected behavior

I think the design makes sense to clear the pool but it could lead to performance issues in case the poolsize is not initialized at the start like CachingSessionFactory<LsEntry> csf = new CachingSessionFactory<LsEntry>(sf, connectionInfo.getPoolSize()); . There could be a fix or this needs to be documented in code so people take care in case of upgrades.

Sample

      CachingSessionFactory<LsEntry> csf = new CachingSessionFactory<LsEntry>(sf ); 
       scsf.setPoolSize(connectionInfo.getPoolSize());
      // Refer https://github.com/spring-projects/spring-integration/pull/2606
      csf.setTestSession(true);
      csf.setSessionWaitTimeout(props.getSftp()
            .getSessionWaitTimeout());
@gauravbrills gauravbrills added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Jun 24, 2022
@artembilan artembilan added in: core backport 5.5.x and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Jun 24, 2022
@artembilan artembilan added this to the 6.0.0-M4 milestone Jun 24, 2022
@artembilan
Copy link
Member

I see your point, @gauravbrills .

If you have a possible fix in mind, feel free to contribute it or share over here.
I will think how that while (delta < 0) { algorithm could be changed...

@gauravbrills
Copy link
Author

Sure we were working on patching our code but I will add PR once I get time next week

artembilan added a commit to artembilan/spring-integration that referenced this issue Jun 27, 2022
Fixes spring-projects#3826

By default, the `SimplePool` is used with an `Integer.MAX_VALUE` pool size.
There is a performance degradation in the `setPoolSize()` when we try to
decrease the pool size: the `while` for `permits.tryAcquire()` is too long
close to the current `Integer.MAX_VALUE` pool size

* Revise the logic in the `setPoolSize()` to use `Semaphore.reducePermits()`
instead of the loop.
* Change the calculation for a new pool size for the current pool state:
or it is a size of a new request, or iti s equal to the `inUse.size()`.
It will be reduced on subsequent `releaseItem()` calls
* Reduce the number of `available` according a new pool size based on the `inUse`.
So, if `inUse > newPoolSize`, the `available` is cleared.
Otherewise, it is reduced to the number which would give `newPoolSize` together
with the `inUse` size

**Cherry-pick to `5.5.x`**
garyrussell pushed a commit that referenced this issue Jun 27, 2022
Fixes #3826

By default, the `SimplePool` is used with an `Integer.MAX_VALUE` pool size.
There is a performance degradation in the `setPoolSize()` when we try to
decrease the pool size: the `while` for `permits.tryAcquire()` is too long
close to the current `Integer.MAX_VALUE` pool size

* Revise the logic in the `setPoolSize()` to use `Semaphore.reducePermits()`
instead of the loop.
* Change the calculation for a new pool size for the current pool state:
or it is a size of a new request, or iti s equal to the `inUse.size()`.
It will be reduced on subsequent `releaseItem()` calls
* Reduce the number of `available` according a new pool size based on the `inUse`.
So, if `inUse > newPoolSize`, the `available` is cleared.
Otherewise, it is reduced to the number which would give `newPoolSize` together
with the `inUse` size

**Cherry-pick to `5.5.x`**
garyrussell added a commit that referenced this issue Jun 27, 2022
Must not go negative.
garyrussell added a commit that referenced this issue Jun 27, 2022
Must not go negative.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants