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 memory leak on authentication error #228

Closed beardy247 closed 4 years ago

beardy247 commented 4 years ago

I believe the root cause of this is in the jCIFS library but this mitigates the problem.

beardy247 commented 4 years ago

@GregBragg @artembilan I think this and my previous fix are really only papering over an issue in the jCIFS library. I've debugged through it endless times and can't get to the bottom of it, the problem is somewhere in SmbTransportImpl or its base class but it is multi threaded and spawns recursive threads so it beat me I'm afraid.

beardy247 commented 4 years ago

Note: I didn't include the upgrade to jcifs-2.1.18 in this commit but that is the version that I'm using.

GregBragg commented 4 years ago

@beardy247 Ok thanks, looks good! You can commit the changes to upgrade the jCIFS library to the latest release which as of yesterday is jcifs-2.1.19 in the Gradle build.gradle file if you want. I'm good with the changes.

beardy247 commented 4 years ago

@GregBragg I've upgraded to 2.1.18 as per your message about 2.1.19 not being in the maven repo yet. Not sure why the message isn't in the thread above.

GregBragg commented 4 years ago

@beardy247 yeah sorry, I deleted the message as I couldn't find the library at first in Maven, then I found it.

I reviewed the change that was made from 2.1.18 to 2.1.19 and it's not significant enough for me to ask you to retest with 2.1.19, however I'll leave that up to you if you want to make the extra effort.

The only change was this: https://github.com/codelibs/jcifs/commit/be2dc950404d14af828b14036211d9447146e324 so not sure if that makes any difference in what you were seeing with the memory leaks.

GregBragg commented 4 years ago

I don't think it would hurt Artem. The way that the URLStreamHandler was instantiated before in version 2.1.18 was suspect anyways, I like the fix they did from 2.1.18 to 2.1.19, so I think we should go with the latest build which only came out yesterday.

beardy247 commented 4 years ago

I've tested with 2.1.19 and the change is inconsequential to our usage.

GregBragg commented 4 years ago

@beardy247 Thanks a bunch!

beardy247 commented 4 years ago

@artembilan I'm using Spring Integration 5.3.0.RELEASE via spring-boot-starter-integration with spring boot 2.3.0.RELEASE. It would be great to get a release a soon as possible, that way I can strip out the code I've put in my project to implement these fixes without having to get the rest of the team to build 1.2.1.BUILD-SNAPSHOT

beardy247 commented 4 years ago

I've got some additional stuff to contribute in the future, I've written a DSL wrapper for the SmbInboundFileSynchronizer and a SmbLastModifiedFileListFilter. I can't submit them yet though as I haven't had change to write unit tests for them. When I find some time I'll get the tests written and a pull request sorted.

artembilan commented 4 years ago

@beardy247 , @GregBragg ,

spring-integration-smb-1.2.1.RELEASE is on its way to Maven Central.