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

Enable TCP keepalive #377

Open
wcs1only opened this Issue Apr 4, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@wcs1only

wcs1only commented Apr 4, 2016

There are some cloud based ftp clients & firewalls that get pretty sloppy about sending FIN packets when killing a long running control connection. When using MultiprocessFTPServer, this means we have a lot of unused processes lying around. I was able to solve this by setting socket.SO_KEEPALIVE in our handler, but I was wondering if you'd accept a patch to the base FTPServer class to enable this by default? Might save somebody a headache or too.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo Apr 4, 2016

Owner

Mmm I am not exactly sure what you mean. By default the client gets
automatically disconnected by the server after 5 minutes of inactivity.
Isn't that enough already? What should SO_KEEPALIVE would accomplish? What
issue are you experiencing exactly?
Il 05/apr/2016 01:03, "Charlie Stanley" notifications@github.com ha
scritto:

There are some cloud based ftp clients & firewalls that get pretty sloppy
about sending FIN packets when killing a long running. When using
MultiprocessFTPServer, this means we have a lot of unused processes lying
around. I was able to solve this by setting socket.SO_KEEPALIVE in our
handler, but I was wondering if you'd accept a patch to the base FTPServer
class to enable this by default? Might save somebody a headache or too.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#377

Owner

giampaolo commented Apr 4, 2016

Mmm I am not exactly sure what you mean. By default the client gets
automatically disconnected by the server after 5 minutes of inactivity.
Isn't that enough already? What should SO_KEEPALIVE would accomplish? What
issue are you experiencing exactly?
Il 05/apr/2016 01:03, "Charlie Stanley" notifications@github.com ha
scritto:

There are some cloud based ftp clients & firewalls that get pretty sloppy
about sending FIN packets when killing a long running. When using
MultiprocessFTPServer, this means we have a lot of unused processes lying
around. I was able to solve this by setting socket.SO_KEEPALIVE in our
handler, but I was wondering if you'd accept a patch to the base FTPServer
class to enable this by default? Might save somebody a headache or too.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#377

@wcs1only

This comment has been minimized.

Show comment
Hide comment
@wcs1only

wcs1only Apr 4, 2016

That was not the behavior I was observing. When using MultiprocessFTPServer, FTP connections that were not closed by the client (dropped by the AWS firewall) stayed in 'ESTABLISHED' state indefinitely, and the corresponding forked process never went away. They'd pile up until we hit max_cons and then no new connections could be served. When I enabled SO_KEEPALIVE, the connections would be closed by the OS and then cleaned up properly.

Perhaps the issue is that I'm using the OS provided version (v1.3.1) which is somewhat older. Is this something that would not appear in later versions?

wcs1only commented Apr 4, 2016

That was not the behavior I was observing. When using MultiprocessFTPServer, FTP connections that were not closed by the client (dropped by the AWS firewall) stayed in 'ESTABLISHED' state indefinitely, and the corresponding forked process never went away. They'd pile up until we hit max_cons and then no new connections could be served. When I enabled SO_KEEPALIVE, the connections would be closed by the OS and then cleaned up properly.

Perhaps the issue is that I'm using the OS provided version (v1.3.1) which is somewhat older. Is this something that would not appear in later versions?

@wcs1only

This comment has been minimized.

Show comment
Hide comment
@wcs1only

wcs1only Apr 4, 2016

These were control connections, not data connections, if that makes any difference.

wcs1only commented Apr 4, 2016

These were control connections, not data connections, if that makes any difference.

@giampaolo

This comment has been minimized.

Show comment
Hide comment
@giampaolo

giampaolo Apr 26, 2018

Owner

This should be due to #447 hence SO_KEEPALIVE should not be necessary.

Owner

giampaolo commented Apr 26, 2018

This should be due to #447 hence SO_KEEPALIVE should not be necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment