fix: Ignore remote peer aborts when accepting clients #165
Conversation
Fixes FTP server becoming unresponsive due to stopped listeners. --- A socket may throw SocketException. This may occur as early as AcceptTcpClientAsync during initial connect hand-shake. With this change, a socket exception with the error code 10054 is ignored during client accept. We do not want to stop the listener altogether when a new client closes their connection. That must only close the client connection. Socket error code 10054: WSAECONNRESET, SocketError.ConnectionReset, 'The connection was reset by the remote peer.' * https://learn.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2 * https://learn.microsoft.com/en-us/dotnet/api/system.net.sockets.socketerror https://learn.microsoft.com/en-us/dotnet/api/system.net.sockets.tcplistener.accepttcpclientasync --- The issue is reproducible in a test scenario of generating mass-connects (my tests had 20 times sets of 400 started concurrently as Tasks). The server will consistently become unresponsive (as in, no longer handle new connection requests but still run as a service as if nothing were wrong) after a few hundred connections and connection closings. I have seen the issue multiple times in production too. Co-authored-by: Jan Klass <jan.klass@ite-si.de>
There was a problem hiding this comment.
Pull Request Overview
This PR ensures the FTP server’s listener ignores remote peer aborts (WSAECONNRESET) during client acceptance to prevent the listener from stopping.
- Add a specific catch for
SocketError.ConnectionResetinAcceptForListenerAsync. - Ensure the listener continues running when a client resets the connection.
- Aligns behavior with existing handling of stopped listeners.
Comments suppressed due to low confidence (1)
src/FubarDev.FtpServer/MultiBindingTcpListener.cs:190
- Add a unit or integration test that simulates a
SocketExceptionwithSocketError.ConnectionResetduringAcceptTcpClientAsyncto verify that the listener remains active and continues accepting new clients.
return new AcceptInfo(null, index);
| @@ -184,6 +184,11 @@ private async Task<AcceptInfo> AcceptForListenerAsync(TcpListener listener, int | |||
| // Ignore the exception. This happens when the listener gets stopped. | |||
| return new AcceptInfo(null, index); | |||
| } | |||
| catch (SocketException ex) when (ex.SocketErrorCode == SocketError.ConnectionReset) | |||
There was a problem hiding this comment.
[nitpick] Update the XML doc comment for AcceptForListenerAsync to explicitly note that a SocketError.ConnectionReset (10054) is intentionally ignored so consumers understand why certain connection resets don’t bubble up.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
There is no XML doc on this method or on this line. Where would I put it? Add a new one on this private method, and then cascade to callers?
IMO, XML doc should match common practice of specifying which exceptions may be thrown, but the lib currently does not do that - does not include this even on exposed interfaces - AFAIK anway.
| @@ -184,6 +184,11 @@ private async Task<AcceptInfo> AcceptForListenerAsync(TcpListener listener, int | |||
| // Ignore the exception. This happens when the listener gets stopped. | |||
| return new AcceptInfo(null, index); | |||
| } | |||
| catch (SocketException ex) when (ex.SocketErrorCode == SocketError.ConnectionReset) | |||
There was a problem hiding this comment.
[nitpick] Consider merging this filter with the existing exception handling to reduce duplication, e.g., using a combined filter: catch (Exception ex) when (ex is ObjectDisposedException || (ex is SocketException se && se.SocketErrorCode == SocketError.ConnectionReset)).
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Both blocks currently have different comments. Those would be hard to represent.
Fixes FTP server becoming unresponsive due to stopped listeners.
A socket may throw SocketException.
This may occur as early as AcceptTcpClientAsync during initial connect hand-shake.
With this change, a socket exception with the error code 10054 is ignored during client accept. We do not want to stop the listener altogether when a new client closes their connection. That must only close the client connection.
Socket error code 10054: WSAECONNRESET, SocketError.ConnectionReset, 'The connection was reset by the remote peer.'
https://learn.microsoft.com/en-us/dotnet/api/system.net.sockets.tcplistener.accepttcpclientasync
The issue is reproducible in a test scenario of generating mass-connects (my tests had 20 times sets of 400 started concurrently as Tasks). The server will consistently become unresponsive (as in, no longer handle new connection requests but still run as a service as if nothing were wrong) after a few hundred connections and connection closings.
I have seen the issue multiple times in production too.