Another "FtpSession - failed to disconnect FTPClient" #3168
Comments
|
This is public void close() {
try {
if (this.readingRaw.get()) {
if (!finalizeRaw()) {
if (this.logger.isWarnEnabled()) {
this.logger.warn("Finalize on readRaw() returned false for " + this);
}
}
}
this.client.logout();
this.client.disconnect();
}
catch (Exception e) {
if (this.logger.isWarnEnabled()) {
this.logger.warn("failed to disconnect FTPClient", e);
}
}
}and this is public void close() {
try {
if (this.readingRaw.get() && !finalizeRaw() && LOGGER.isWarnEnabled()) {
LOGGER.warn("Finalize on readRaw() returned false for " + this);
}
if (this.client.isConnected()) {
this.client.logout();
}
this.client.disconnect();
}
catch (Exception e) {
LOGGER.warn("failed to disconnect FTPClient", e);
}
}So, technically there is no difference: that On the other side it is just a Therefore I don't see how the mentioned fix for #3123 may affect you. I mean you don't see that warn in the version Consider just to ignore such a WARN message in your logs or let us know that it would be better to move it into an I don't see anything wrong with the logic we have so far therefore not sure what is your idea about a bug. Thank you for understanding! |
artembilan
added
status: waiting-for-reporter
|
I don't think changing logging levels is right since we don't yet know root cause. Key issue is why have these new Best I can tell is that the new If you want me to set specific loggers to |
|
Interesting; looking at the Apache code, I don't see any side effects of calling So the only side effect would be some tiny timing differences. So, I am not sure why you would see new exceptions there. However, it does look like we should try/catch around the logout so we don't skip the |
|
Yes, please, The |
Yeah... Agree. But that is not the point. @jonforums just doesn't see a WARN in previous version. |
I understand that; I just don't see a new path to getting these exceptions. Broken pipe when sending the QUIT implies the server reset the connection; makes no sense to me. |
|
Will set Also reading 3rd para of https://commons.apache.org/proper/commons-net/apidocs/org/apache/commons/net/ftp/FTP.html talking about FTP servers closing idle client connections. |
Interesting; in both cases, the Seems odd that the |
|
Is there a more specific logger I can set to |
|
Try |
|
Interesting to see the time delta between the first successful batch of FTP sends, the second batch of failed FTP sends, and Maybe there's a code path in In between the above successful FTP send of two files and the following FTP send failure, (2) good SFTP sends occurred using |
|
So, I think we need to change that Does it sound reasonable? |
|
Sounds reasonable to me, but I still don't understand why this is a "new" problem on 5.2.x. I suggest we also add a DEBUG or TRACE log when we get an exception is |
Yeah... That's really weird. Doesn't look like we have done some changes about caching in between. @jonforums , any chances to to have some investigation on your side from the version Thanks |
|
After spelunking SI's FTP, session, pool, and remote code more, I also don't see why this is a "new" 5.2.x problem...5.2.x vs. 5.1.9 now seems like a red herring. I'll spelunk a few log archives from 5.1.9 days to see if a similar exception was in the logs and I missed them back then. |
|
Over a 5 week period (last 2 weeks of 2019, first 3 weeks of 2020) no FtpSession While I also don't think it's a 5.1.9 vs. 5.2.3 issue, it is odd that over 5 weeks of prod usage I didn't see the FtpSession warning. I know you're trying to determine if/where to backport. If you think it will meaningfully help, let me know if you'd like me to try to repro on 5.1.9. I'd try to quickly configure a long-running MINA FtpServer v1.1.1 process on my windows dev laptop to see what happens. Not apples-to-apples with my Ubuntu Server/vsftpd prod setup, but another data point if you need it. |
|
Thanks. That might be useful to observe similar logs during processing to determine a difference. |
|
Repro'd on both 5.2.3 and 5.1.9 on my windows dev laptop using ftpserv v1.1.1. The test scenario for both was: FTP send (2) files, wait (5) minutes, SFTP send (1) file, wait (30) minutes, FTP send (1) file. In both 5.2.3 and 5.1.9, during the last FTP send I can provide logs for both 5.2.3 and 5.1.9 if needed, but here's the relevant log snippet from the 5.1.9 run: |
|
So, therefore there is no difference between those versions and we are good to go with Or should we treat this as "Works as Designed" and close respectively? What I see that you confirm that there is no regression and behavior is the same. |
|
Whatever you decide, I'd like to see this specific class of |
|
Got it! I'll ping you for PR review. Or let me know if you are good with contributing the fix: https://github.com/spring-projects/spring-integration/blob/master/CONTRIBUTING.adoc |
|
I'm not going to go through the contribution ritual. It's enough to see that you two are comfortable with a reliable fix, and updates are on the way to my favorite Spring family project 😃 My only remaining concern is with integration tests. I don't believe the intermediate SFTP send in my test scenario had anything to do with the problem. I think it was caused by the ~35 minute (??) period between the first FTP sends and the second FTP send. It would be nice to have longer running integration tests that don't run too long but still catch these types of scenarios. Maybe the MINA-based ftpserver and sshserver libraries have an easy-to-configure option to set client disconnects to ~2-3 minutes. |
Fixes spring-projects#3168 * Call `this.client.noop()` instead of `this.client.isConnected()` to really check that client has a live connection with the server before calling a `this.client.logout()` * Wrap `this.client.logout()` into a `try..catch` to be sure that we wil call a `this.client.disconnect()` even if `logout()` fails for some reason. * Change `WARN` logs about `Session.close()` into a `DEBUG` level - when we have a problem with closing session because of server disconnect reason we have no any control to do with a situation **Cherry-pick to 5.2.x**
Fixes #3168 * Call `this.client.noop()` instead of `this.client.isConnected()` to really check that client has a live connection with the server before calling a `this.client.logout()` * Wrap `this.client.logout()` into a `try..catch` to be sure that we wil call a `this.client.disconnect()` even if `logout()` fails for some reason. * Change `WARN` logs about `Session.close()` into a `DEBUG` level - when we have a problem with closing session because of server disconnect reason we have no any control to do with a situation **Cherry-pick to 5.2.x**
Fixes #3168 * Call `this.client.noop()` instead of `this.client.isConnected()` to really check that client has a live connection with the server before calling a `this.client.logout()` * Wrap `this.client.logout()` into a `try..catch` to be sure that we wil call a `this.client.disconnect()` even if `logout()` fails for some reason. * Change `WARN` logs about `Session.close()` into a `DEBUG` level - when we have a problem with closing session because of server disconnect reason we have no any control to do with a situation **Cherry-pick to 5.2.x**
Affects Version(s): 5.2.3
Recently upgraded from 5.1.9 to 5.2.3 and now seeing FTP warnings that never happened with 5.1.9. My issue appears similar to #3123.
Relevant config is the following. Spring Integration code is interacting with a vsftpd server running on Ubuntu Server.
The following is the most common exception with multiple occurrences:
However, there is one occurrence of this exception
The text was updated successfully, but these errors were encountered: