FTP Session not correctly closed

[cosenmarco]

Affects Version(s): \ 4.3.20-RELEASE

Hi, we're using spring-integration for uploading FTP files to a server of ours.
Due to some unrelated problem we took a TCP dump on the FTP traffic and we could identify an issue in spring-integration which I'd like to report here.

Basically the FTP session is not closed at all but just the connection is closed. See

spring-integration/spring-integration-ftp/src/main/java/org/springframework/integration/ftp/session/FtpSession.java

Line 146 in 3cd751a

public void close() {

This causes the following interaction

I would suggest to add method call logout() in the Session's close() implementation.

[artembilan]

Looks like you are right: https://www.journaldev.com/661/java-ftp-client-upload-example-apache-commons-net

I really see there the mentioned logout() call.

The same is also mentioned in the FTPClient JavaDocs: https://commons.apache.org/proper/commons-net/apidocs/org/apache/commons/net/ftp/FTPClient.html
Same logout() is used in this article as well: https://examples.javacodegeeks.com/core-java/apache/commons/net-commons/ftp/ftpclient/org-apache-commons-net-ftp-ftpclient-example/

So, I consider this as a bug and we'd be glad to get a fix from you: https://github.com/spring-projects/spring-integration/blob/master/CONTRIBUTING.adoc !

Thank you for bringing this up!
That's really surprising how we haven't caught this issue for all these years of that implementation...
Maybe most of the FTP servers close sessions as well when we disconnect?

[cosenmarco]

Hi artembilan,
thanks for confirming my hypothesis.

Maybe most of the FTP servers close sessions as well when we disconnect?

That's the behaviour expected anyway in the RFC:

An unexpected close on the control connection will cause the
server to take the effective action of an abort (ABOR) and a
logout (QUIT).

But it definitely is not a clean shutdown. Also I'm not very happy with how the client forcibly ties to close the connection even though the server is trying to communicate that there's something fishy. That is though to attribute to the Apache client which I suppose just closes the socket and lets the OS deal with whatever related packet comes from the other party.

I'll have to see if I get some time to contribute a solution. The solution may be trivial as I suggested but the question is (as always) how to test it. Is there any suggestion on how to get this covered besides adjusting the existing tests (which I didn't give a look yet) ?

[artembilan]

Yeah... I would say let's proceed with the solution and see if all our existing tests are passing!
In addition you can test the solution with your environment to be sure that there is no resource leaking any more.
I don't see how valuable a complex integration test can be in the Framework.
More over we should test here our own components, but with the mentioned logic it looks more like Apache FTP Client concern: we just have to follow their requirements and really call that logout(). Nothing we can cover with lightweight test without exposing that FTP Client internals. Or server side...

So, let's proceed just with the code change and manual testing! Hope your environment can prove the main argument of this issue.

Thanks

[cosenmarco]

I don't see how valuable a complex integration test can be in the Framework.

I tend to disagree a bit on this point.
The level of details exposed by the Apache client regarding the protocol is quite high and the responsibility of conducting a proper session is delegate to the user (in our case the FTPSession class) which should guarantee to it users (the users of spring-integration) that the session is conducted according to standards.

If I had to implement this at work, I would try to find a mock FTP server that allows me to make assertions about the workflow and simulate various scenarios.

Again I didn't look at the existing tests but I really don't have time right now to dig into this as at work is not a critical issue. I may spend some of my private time on this but that may happen in weeks, rather than days.

[artembilan]

Well, we use Apache Mina FtpServer for testing in the unit tests of the project. There is an FtpTestSupport super class to use a common server configuration in the target tests.
However I would just made a simple mock test against FTPClient to verify that its logout() is called when we execute FtpSession.close().
Of course, we would need to be sure that the whole existing test suite for FTP still passes.

But since you can't make a fix in time for next Monday release, we take care of this ourselves.

Even if you can't make PR right now, your comments on this are fully enough to treat as a contribution.

We really respect your working time and appreciate any feedback whenever it happens.

Thank you again!