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
Fix constant flushing in FTPSocket
  • Loading branch information
@ViRb3
ViRb3 committed Oct 3, 2025
commit c79862d262b8b9a849167314d50b1b8683bf2640
63 changes: 41 additions & 22 deletions 63 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 @@ -17,14 +17,13 @@
* Bug fixes, suggestions and comments should be sent to feedback@cyberduck.ch
*/

import ch.cyberduck.core.ConnectionTimeout;
import ch.cyberduck.core.ConnectionTimeoutFactory;
import ch.cyberduck.core.preferences.PreferencesFactory;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import java.io.FilterInputStream;
import java.io.FilterOutputStream;
import java.io.BufferedInputStream;
import java.io.BufferedOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand All @@ -51,15 +50,15 @@ public class FTPSocket extends Socket {
private static final Logger log = LogManager.getLogger(FTPSocket.class);

private final Socket delegate;

private final ConnectionTimeout connectionTimeoutPreferences = ConnectionTimeoutFactory.get();
private InputStream inputStreamWrapper;
private OutputStream outputStreamWrapper;

public FTPSocket(final Socket delegate) {
this.delegate = delegate;
}

@Override
public void close() throws IOException {
public synchronized void close() throws IOException {
if(delegate.isClosed()) {
log.debug("Socket already closed {}", delegate);
return;
Expand Down Expand Up @@ -93,7 +92,7 @@ else if(!delegate.isConnected()) {
}
finally {
log.debug("Closing socket {}", delegate);
// Work around macOS bug where Java NIO's SocketDispatcher.close0() has a 1,000ms delay
// Work around macOS quirk 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();
Expand Down Expand Up @@ -136,23 +135,43 @@ public SocketChannel getChannel() {
}

@Override
public InputStream getInputStream() throws IOException {
return new FilterInputStream(delegate.getInputStream()) {
@Override
public void close() throws IOException {
FTPSocket.this.close(); // Call wrapper's close, not delegate's
}
};
public synchronized OutputStream getOutputStream() throws IOException {
if(outputStreamWrapper == null) {
outputStreamWrapper = new BufferedOutputStream(delegate.getOutputStream(),
dkocher marked this conversation as resolved.
Outdated Show resolved Hide resolved
PreferencesFactory.get().getInteger("connection.buffer")) {
@Override
public void close() throws IOException {
try {
// We can't close this stream as it would propagate to delegate.close()
// Therefore, we must flush before we manually close the delegate
super.flush();
dkocher marked this conversation as resolved.
Show resolved Hide resolved
}
catch(IOException e) {
log.error("Error flushing output stream for socket {}: {}", delegate, e.getMessage());
ViRb3 marked this conversation as resolved.
Outdated Show resolved Hide resolved
}
finally {
// Stream close will call Socket.close(), so override it with ours
FTPSocket.this.close();
}
}
};
}
return outputStreamWrapper;
}

@Override
public OutputStream getOutputStream() throws IOException {
return new FilterOutputStream(delegate.getOutputStream()) {
@Override
public void close() throws IOException {
FTPSocket.this.close(); // Call wrapper's close, not delegate's
}
};
public synchronized InputStream getInputStream() throws IOException {
if(inputStreamWrapper == null) {
inputStreamWrapper = new BufferedInputStream(delegate.getInputStream(),
ViRb3 marked this conversation as resolved.
Outdated Show resolved Hide resolved
PreferencesFactory.get().getInteger("connection.buffer")) {
@Override
public void close() throws IOException {
// Stream close will call Socket.close(), so override it with ours
FTPSocket.this.close();
}
};
}
return inputStreamWrapper;
}

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