search

spring-projects / spring-integration-extensions

The Spring Integration Extensions project provides extension components for Spring Integration
http://www.springintegration.org/
280 stars 266 forks source link

Fix for memory leak issue #225 #227

Closed beardy247 closed 4 years ago

beardy247 commented 4 years ago

If SmbShare instantiates a BaseContext during construction then the context will be closed on calling close().

Without this there's a ThreadLocal memory leak.

beardy247 commented 4 years ago

Hi @artembilan, I've deleted the previous branch and fixed the issue with line endings in git/eclipse that caused the issue with the old pull request. I've checked this thoroughly with JProfiler and I'm now satisfied that the leak when polling is gone. Thanks, Adam

artembilan commented 4 years ago

Thank you for contribution ; looking forward for more!

beardy247 commented 4 years ago

When's the next release scheduled?

GregBragg commented 4 years ago

@artembilan we might want to upgrade the base jCIFS library to the latest release before we create a new build as we are 7 minor,minor versions behind.

@beardy247 maybe you might want to reviewed these changes to the base library: https://github.com/codelibs/jcifs/compare/jcifs-2.1.11...jcifs-2.1.18 and retest with JProfiler?

I see a few fixes for "Fix session/connection leak on authentication errors" and "Lazily remove connections from pool to avoid deadlocks" that might resolve further memory/thread issues.

artembilan commented 4 years ago

Yes, we can release at any moment. Let's really see if the latest jCIFS is great, upgrade to the latest deps and release!

beardy247 commented 4 years ago

@GregBragg I'll take a look, I've got a few days left on my JProfiler trial. I did spot the leak on authentication errors, but forgot to pursue it as I was focusing on the main leak as it was a major blocker to the progress of our project.

GregBragg commented 4 years ago

Awesome! Thanks for this, really appreciate the extra effort!

beardy247 commented 4 years ago

@GregBragg I've done some profiling and I was all but convinced there was still a leak in the jCIFS library on authentication errors but on closer inspection I think the library is OK. To make it easier to spot leaks in JProfiler I'd taken out the CachingSessionFactory from my code so that each poll created a new session (with invalid credentials) with a polling period of 10 seconds, I think the leak stems from the default timeout in SmbTransportImpl being 30 seconds, so it's creating sessions faster than it's tidying up after itself. The answer may be just to call getContext().close() in the catch block of SmbShare.init(). What do you think?

beardy247 commented 4 years ago

@GregBragg Having looked again at this this morning, I think the timeout is a red herring. I've submitted a new pull request with a fix that protects spring integration from the leak.

beardy247 commented 4 years ago

I've reviewed the other changes to the base library and they all look OK.

GregBragg commented 4 years ago

@beardy247 Thanks for your contribution! It looks great.