Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Shutdown socket in some cases to prevent endless hang on blocking operation#32087

Closed
wfurt wants to merge 14 commits intodotnet:masterfrom
wfurt:blocking_socket
Closed

Shutdown socket in some cases to prevent endless hang on blocking operation#32087
wfurt wants to merge 14 commits intodotnet:masterfrom
wfurt:blocking_socket

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Sep 4, 2018

reopening after #26898 was closed.

Fixes #22564
Fixes #26034

@wfurt wfurt requested a review from a team September 4, 2018 06:06
{
if ((AsyncContext == null || !AsyncContext.GetNonBlocking()) && innerSocket != null && !_underlyingHandleNonBlocking && !innerSocket.IsClosed && !innerSocket.IsInvalid)
{
// We only need to coll this for true blocking calls when there is chance they are stuck in OS system call.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: coll

@karelz karelz added this to the 3.0 milestone Sep 6, 2018

public class BlockingSendReceive
{

Choose a reason for hiding this comment

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

Nit: remove blank line

server.BindToAnonymousPort(ipAddress);
server.Listen(1);
byte[] buffer = new byte[1];
buffer[0] = Convert.ToByte('a');

Choose a reason for hiding this comment

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

I don't think you need to use Convert here, you can just cast to byte, i.e. (byte)'a' -- we do this in product code in some places.

buffer[0] = Convert.ToByte('a');
server.BindToAnonymousPort(ipAddress);
server.Listen(1);
Task clientTask = Task.Run(() =>

Choose a reason for hiding this comment

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

Seems like this should be called "serverTask" (and in the comment below)

}

[Theory]
[PlatformSpecific(~TestPlatforms.OSX)]

Choose a reason for hiding this comment

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

Is there a separate issue for OSX? Can we link the issue here?

Copy link
Member Author

Choose a reason for hiding this comment

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

not at the moment, both issues were reported on Linux. I can update #26034 and leave it open for OSX instead of closing.

// Blocking read.
try
{
client.Receive(buffer);

Choose a reason for hiding this comment

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

What is the expected behavior here when we Close? Do we throw? Do we return 0 bytes? Seems like we should validate the expected behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will throw one or the other exception in catch. It it throws something else or if it hangs, test should fail.

// Each round will block until new connection is accepted.
while (true)
{
Socket serverSocket = server.Accept();

Choose a reason for hiding this comment

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

What is the expected behavior here when we Close? Do we throw? Do we return null?

Choose a reason for hiding this comment

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

As above, we should verify the expected behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

same as comment above.

@wfurt
Copy link
Member Author

wfurt commented Sep 24, 2018

also fixes #31368. New test added.

@davidsh davidsh requested a review from stephentoub September 24, 2018 19:48
@wfurt
Copy link
Member Author

wfurt commented Oct 1, 2018

@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop OSX x64 Debug Build
@dotnet-bot test Outerloop Windows x64 Debug Build

@karelz
Copy link
Member

karelz commented Dec 19, 2018

Not touched in a long time, closing for now. Feel free to reopen when it is ready.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TcpListener.Stop hangs if Accept is in progress Linux Sockets: Close with pending synchronous Receive hangs

6 participants