Skip to content
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

Support NOOP and NoopInterval for keeping control socket alive during long file transfers #504

Merged

Conversation

aliquid
Copy link

@aliquid aliquid commented Jan 4, 2020

This PR adds support for sending NOOP commands every 15 seconds (or whatever value you've set NoopInterval) in order to keep the control socket alive during long file transfers.

It fixes the long standing issue: Timed out trying to read data from the socket stream!

Related PR: #507

Fixes issues: #122, #387, #453, #297, #293, #256, #253, #245, #244, #156, #55


It seems that the AzureVM router or firewall does not respect TCP Keep-Alive on the control socket (port 21). When large file transfers runs on data socket (20 or passive) there is no traffic on the control socket and it breaks for us after ~280s (perhaps 240s). Before mentioned workaround setting EnableThreadSafeDataConnections = true simply caused the exception from the broken connection to be discarded, pretending all was fine and opening a new control socket.

The NOOP command responds something like '200 NOOP command successful.' and is typically used by a FTP UI client to keep the control socket alive in-between commands, so it is a good candidate for using also during long-running command executios (i.e. RETR/STOR). However the https://tools.ietf.org/html/rfc959 neither suggests that nor against it.

Indeed, googling shows that it has been proven in battle, with some odd complaints (see link to smartftp.com some comments back) that it could potentially cause file transfers to abort (remains to be proven) or make FTP servers behave strangely like not responding 200 (that is what I can see from IIS) or respond at the end of the file transfer (have no such evidence, but pull request attempted to cope with that in a rudimentary manner).

Suggested usage for NoopInterval (in milliseconds) from our experience;

  • 0 (default) if you can get away with it do not fiddle with it, keeping in mind that sending commands during command execution might not be by the book for all FTP server implementations out there
  • firewall timeout divided by 2, if you can determine the timeout by transferring files 10MB, 50MB, 100MB, 500MB, 1GB, 5GB
  • 30000 or 45000 or 90000 (corresponding to 30s, 45s or 90s) would make sense
  • not something like 1000 or 5000, that would be nonsensical because any socket must survive 5s silence, problem should be sought elsewhere like in the firewall

@aliquid aliquid closed this Jan 4, 2020
@aliquid aliquid reopened this Jan 4, 2020
@aliquid
Copy link
Author

aliquid commented Jan 4, 2020

Ignoring Codacy/PR Quality Review complaint in favor of conformity with existing code.

Complaint;

		private int m_noopInterval = 0;

Existing code;

		private int m_port = 0;
...
		private double m_timeDiff = 0;
...
		private uint m_uploadRateLimit = 0;
...
		private uint m_downloadRateLimit = 0;

@robinrodricks
Copy link
Owner

This is awesome. It's been a long outstanding issue and I've literally had no idea how to fix it. I'll review it the moment I get some time. Thanks for your effort/PR. Kudos and wish you a Happy New year!

@aliquid
Copy link
Author

aliquid commented Jan 4, 2020

That should be the last commit from me. Looking forward to the next release! Meantime we are good with the workaround of setting EnableThreadSafeDataConnections = true. Thank you!

Edit; please squash when merging :-)

@robinrodricks
Copy link
Owner

This is very well done indeed. Thank you again kind sir :)

@robinrodricks robinrodricks merged commit f04a307 into robinrodricks:master Jan 6, 2020
@robinrodricks
Copy link
Owner

Can we set a default value for NoopInterval so its enabled by default? I don't see how it can hurt anything. What value should I keep? 15000?

@robinrodricks
Copy link
Owner

Done. Gone live with 29.0.0:

  • New: Keep control socket alive during long file transfers using NOOP (thanks aliquid)
  • New: Add NoopInterval property to control interval of NOOP commands (thanks aliquid)

https://www.nuget.org/packages/FluentFTP/29.0.0

@aliquid aliquid deleted the features/387-NoopInterval branch January 6, 2020 13:26
@robinrodricks robinrodricks changed the title #387 support for NoopInterval for keeping control socket alive during file transfers Support NOOP and NoopInterval for keeping control socket alive during file transfers Jan 6, 2020
@robinrodricks robinrodricks changed the title Support NOOP and NoopInterval for keeping control socket alive during file transfers Support NOOP and NoopInterval for keeping control socket alive during long file transfers Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants