Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Fix race conditions in FTP socket closure that cause intermittent errors #17540

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
Loading
from
+137 −1
Open

Fix race conditions in FTP socket closure that cause intermittent errors #17540

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e571e75
Fix race conditions in FTP close
ViRb3 Oct 3, 2025
e84e5ee
Improvements in FTPSocket
ViRb3 Oct 3, 2025
c79862d
Fix constant flushing in FTPSocket
ViRb3 Oct 3, 2025
ad035ed
Replace buffered with proxy streams in FTPSocket
ViRb3 Oct 5, 2025
b227e01
Surface critical errors in FTPSocket
ViRb3 Oct 5, 2025
352e11c
Fix read amplification in FTPSocket
ViRb3 Oct 5, 2025
47b7033
Refactor FTPSocket to address comments
ViRb3 Oct 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Improvements in FTPSocket
  • Loading branch information
@ViRb3
ViRb3 committed Oct 3, 2025
commit e84e5ee10f888ad9c0d12f0e8e100f8562ae6cb5
50 changes: 27 additions & 23 deletions 50 ftp/src/main/java/ch/cyberduck/core/ftp/FTPSocket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.net.SocketAddress;
import java.net.SocketException;
import java.nio.channels.SocketChannel;
import java.util.concurrent.CompletableFuture;

/**
* Socket wrapper that enforces proper TCP shutdown sequence to prevent
Expand Down Expand Up @@ -59,9 +60,16 @@ public FTPSocket(final Socket delegate) {

@Override
public void close() throws IOException {
if(delegate.isClosed()) {
log.debug("Socket already closed {}", delegate);
return;
}
try {
if(delegate.isClosed() || delegate.isOutputShutdown() || !delegate.isConnected()) {
log.debug("Socket already closed {}", delegate);
if(delegate.isOutputShutdown()) {
log.debug("Socket output already closed {}", delegate);
}
else if(!delegate.isConnected()) {
log.debug("Socket is already disconnected {}", delegate);
}
else {
// Shutdown output to send FIN, but keep socket open to receive ACKs
Expand All @@ -70,34 +78,30 @@ public void close() throws IOException {

// Wait for server FIN by draining any remaining data
// This ensures all our data packets are ACKed before closing
try {
// Avoid hanging in case the server malfunctions
delegate.setSoTimeout(connectionTimeoutPreferences.getTimeout() * 1000);
InputStream in = delegate.getInputStream();
byte[] buffer = new byte[8192];
int bytesRead;
// Read until EOF (server closes its side) or timeout
while((bytesRead = in.read(buffer)) != -1) {
log.debug("Drained {} bytes from socket after shutdown", bytesRead);
}
log.debug("Received EOF from server, all data acknowledged");
}
catch(java.net.SocketTimeoutException e) {
// Timeout is acceptable - server may not close its side
log.debug("Timeout waiting for server EOF, proceeding with close");
}
catch(IOException e) {
// Other errors during drain are acceptable
log.debug("Error draining socket: {}", e.getMessage());
log.debug("Draining input for socket {}", delegate);
InputStream in = delegate.getInputStream();
byte[] buffer = new byte[8192];
int bytesRead;
// Read until EOF (server closes its side) or timeout
while((bytesRead = in.read(buffer)) != -1) {
log.debug("Drained {} bytes from socket {}", bytesRead, delegate);
}
}
}
catch(IOException e) {
log.warn("Failed to shutdown output for socket {}: {}", delegate, e.getMessage());
log.error("Failed to shutdown output for socket {}: {}", delegate, e.getMessage());
}
finally {
log.debug("Closing socket {}", delegate);
delegate.close();
// Work around macOS bug where Java NIO's SocketDispatcher.close0() has a 1,000ms delay
CompletableFuture.runAsync(() -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running in a background thread and missing any error looks dangerious to me as we will not report any error when closing the stream fails which may indicate that the file is not properly persisted on the remote server.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree. I tried really hard to find a workaround, but couldn't come up with one. Having 1 whole second delay between each file upload is really bad when you upload lots of small files, and it causes Cyberduck to be upwards of 10x slower than competitor FTP clients. If you have any suggestion here, I'm more than happy to take it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I don't think this failing specifically could result in data loss. By the time we've reached here we already flushed our data, closed our stream, and waited for the server to close its stream. So, I don't think failing here could ever result in data loss.

The only case of data loss should occur if "Failed to shutdown output for socket" is hit. Maybe we can change that to throw instead of logging and discarding.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are probably right there is no risk here in the real world. But it should probably not be in the finally block as it does not need to be executed when above code fails with I/O.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that close quirk on macOS specific to server implementations?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't want to include the workaround if it can only be reproduced with a single FTP server implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other FTP client experiences this delay though, so it's clearly on Cyberduck's side. I'm away from home this week but will look more into it next week.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkocher I did a deep dive on this.

  1. I compared Forklift's traffic dump with Cyberduck + this PR, and it is completely identical. Packets are the same and their options (SYN,ACK,FIN) too.
  2. I debugged the JVM native code and traced the 1s delay to macOS close() syscall.
  3. I wrote a script to dump netstat -anl to file and confirmed that in both apps, the socket transitions from SYN_SENT -> ESTABLISHED -> FIN_WAIT_1 -> FIN_WAIT_2 -> TIME_WAIT. Forklift remains in TIME_WAIT for 5s+, while Cyberduck for 1-5s+ depending on SO_LINGER flag. I'm unsure why SO_LINGER(true, 0) results in 1s of TIME_WAIT, as I thought that it should be skipped entirely? This could be a quirk in netstat on macOS though.
  4. I tried to reproduce this issue using a minimal TCP client/server in C, but it does not block there regardless of SO_LINGER.
  5. The only difference between the "1s hang" server and "no hang" server is that the "1s hang" server completes 4-way shutdown of data socket first, and then sends "OK" on control socket, while the "no hang" server sends "OK" on control socket first, and then sends the FIN on data socket. This, and also some packet timing differences.

If the control socket is truly independent of the data socket as I understand it is, then my conclusion is that Socket.close() blocking is purely due to timing. There should be nothing preventing it from happening on any FTP server given that it hits the same timing.

So, unless you have better ideas, I vote to keep the current workaround.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just one more suggestion to try out to see what difference it makes with different SO_LINGER options:

  • Are you testing with SSL sockets? We enabled SO_LINGER for this use case if I remember correctly 1
  • What is the behaviour with connection.socket.linger set to false in default.properties or explicitly in DefaultSocketConfigurator
  • Enable SO_LINGER but modify DefaultSocketConfigurator to set with a shorter timeout value of 1. With 0 I see 426 errors for data transfers. 2

Footnotes

  1. 94718d5 ↩

  2. 6c2d19f ↩

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have opened #17562 to make the SO_LINGER timeout configurable.

try {
delegate.close();
}
catch(IOException e) {
log.error("Error closing socket {}: {}", delegate, e.getMessage());
}
});
}
}

Expand Down
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.