Remove calls to Socket.Poll and use SocketShutdown.Both #1706
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The message loop currently sits in a call to Poll until the socket has data to read or it is closed. This is unnecessary - it can equally just sit in the call to Receive. The call to Poll in Session.IsConnected is also unnecessary - we can instead just call Socket.Connected. This only returns the connection state as of the last operation, but we are always performing operations in the message loop (or else we are not connected), so it should work equally well while being cheaper. Lastly, when shutting down the socket, shut down both sides rather than just the sending side (SocketShutdown.Both rather than SocketShutdown.Send) - at this point we do not care about reading anything else. This makes it (more) certain that we will break out of the Receive call in the message loop, as has been noted in sshnet#355 for whatever remaining issues still exist there.
Rob-Hague
requested review from
WojciechNagorski and
drieseng
as code owners
Rob-Hague
commented
Sep 29, 2025
| /// Holds an object that is used to ensure only a single thread can read from | ||
| /// <see cref="_socket"/> at any given time. | ||
| /// </summary> | ||
| private readonly Lock _socketReadLock = new Lock(); |
There was a problem hiding this comment.
no longer needed
| /// </summary> | ||
| private void SocketDisconnectAndDispose() | ||
| { | ||
| if (_socket != null) |
There was a problem hiding this comment.
not a hot path, so simplify the locking and save some indentation
|
|
||
| // any timeout while disconnecting can be caused by loss of connectivity | ||
| // altogether and should be ignored | ||
| if (exp is SocketException socketException && socketException.SocketErrorCode == SocketError.TimedOut) |
There was a problem hiding this comment.
dead code, we always wrap SocketException in SshConnection
Comment on lines
-67
to
-68
| Assert.IsNull(connectionException.InnerException); | ||
| Assert.AreEqual("An established connection was aborted by the server.", connectionException.Message); |
There was a problem hiding this comment.
not always the case - it could contain a SocketException upon RST, in which case the message is more like "An existing connection was forcibly closed by the remote host."
There was a problem hiding this comment.
whitespace off: https://github.com/sshnet/SSH.NET/pull/1706/files?w=1
WojciechNagorski
approved these changes
Oct 1, 2025
|
Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The message loop currently sits in a call to Poll until the socket has data to read or it is closed. This is unnecessary - it can equally just sit in the call to Receive.
The call to Poll in Session.IsConnected is also unnecessary - we can instead just call Socket.Connected. This only returns the connection state as of the last operation, but we are always performing operations in the message loop (or else we are not connected), so it should work equally well while being cheaper.
Lastly, when shutting down the socket, shut down both sides rather than just the sending side (SocketShutdown.Both rather than SocketShutdown.Send) - at this point we do not care about reading anything else. This makes it (more) certain that we will break out of the Receive call in the message loop, as has been noted in #355 for whatever remaining issues still exist there.