Skip to content
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

Suppressing a not-connected exception on socket shutdown #86

Closed
wants to merge 1 commit into from

Conversation

billycc
Copy link

@billycc billycc commented Sep 14, 2016

I'm trying out SshNet on an ARM device with a very old linux kernel (2.6.29) and old version of mono (3.0.11). I'm not sure if this is a function of my ancient environment.

While closing a socket associated with ChannelDirectTcpip, the _socket seems to respond true to _socket.Connected; but throws SocketException: The socket is not connected when it hits _socket.Shutdown(). Since it's a shutdown procedure anyway, I assume this exception is not important so I just send it to the log and escape out.

I ran some limited tests and by suppressing this exception, SshNet no longer bombs out and instead continues to be available and seems to continue humming along without any other apparent issues. I have not checked if there's other sockets which also have an untrustworthy Connected property.

@drieseng
Copy link
Member

@billycc I probably won't accept the PR as is, as we don't want to (and cant't) base exception handling on the message of an exception. I case of a Shutdown, we could probably just ignore - meaning log - any SocketException. That what we do now in Session.SocketDisconnectAndDispose().

I'd prefer to have test coverage for this change too.

@billycc
Copy link
Author

billycc commented Sep 14, 2016

Yeah, I felt dirty writing it. I did filter it to SocketException, but I'm not sure if there's anything else to look for so I wanted to try to be a little more specific than that. It's hell to get anything over onto my arm box so I didn't spend too much time spelunking.

I'll see if I can get a more reasonable/repro-able mono environment together and see if I can make a coverage case. Maybe later in the week.

@drieseng drieseng added this to the 2016.1.0 milestone Sep 18, 2016
@drieseng drieseng added the bug label Sep 18, 2016
@drieseng drieseng self-assigned this Sep 18, 2016
drieseng added a commit that referenced this pull request Sep 18, 2016
@drieseng
Copy link
Member

This is now fixed in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants