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

Tweak AsyncSocketListener in tests #1382

Merged
merged 4 commits into from
May 10, 2024

Conversation

Rob-Hague
Copy link
Collaborator

Some CI runs have been crashing lately from within AsyncSocketListener. This moves a bit of disconnection & exception handling around. It probably won't fix the underlying tests which would have otherwise failed, but it might stop the process from outright crashing.

Also reference the same code for the integration tests.

Some CI runs have been crashing lately from within AsyncSocketListener.
This moves a bit of disconnection & exception handling around. It probably won't
fix the underlying tests which would have otherwise failed, but it might stop the
process from outright crashing.

Also reference the same code for the integration tests.
@@ -322,23 +284,36 @@ void ConnectionDisconnected()
}
catch (ObjectDisposedException)
{
// TODO On .NET 7, sometimes we get ObjectDisposedException when _started but only on appveyor, locally it works
ConnectionDisconnected();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the main problem. If we have received ObjectDisposedException, we don't want to call Shutdown on the socket because we will get another ObjectDisposedException, and that will crash the process.

@WojciechNagorski
Copy link
Collaborator

This is a very difficult piece of code. Sockets will behave differently on Linux and Windows and in the .NET Framework and modern .NET. In addition, different tests use the AsyncSocketListener differently. But I agree that it should be a little easier.

@Rob-Hague
Copy link
Collaborator Author

Yeah, I figured this at least removes one way of crashing. There might still be more

@@ -8,6 +8,10 @@
<AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
</PropertyGroup>

<ItemGroup>
<Compile Include="$(ProjectDir)../Renci.SshNet.Tests/Common/AsyncSocketListener.cs" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great idea 👍

@WojciechNagorski WojciechNagorski merged commit 0cfeb6b into sshnet:develop May 10, 2024
1 check passed
@Rob-Hague Rob-Hague deleted the asyncsocketlistener branch May 10, 2024 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants