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 into
dotnet:masterfrom
wfurt:blocking_socket
Closed

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

Conversation

@wfurt

@wfurt wfurt commented Sep 4, 2018

Copy link
Copy Markdown
Member

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
Copy Markdown
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
{

Copy link
Copy Markdown

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');

Copy link
Copy Markdown

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(() =>

Copy link
Copy Markdown

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)]

Copy link
Copy Markdown

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
Copy Markdown
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);

Copy link
Copy Markdown

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
Copy Markdown
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();

Copy link
Copy Markdown

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?

Copy link
Copy Markdown

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
Copy Markdown
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

wfurt commented Sep 24, 2018

Copy link
Copy Markdown
Member Author

also fixes #31368. New test added.

@davidsh
davidsh requested a review from stephentoub September 24, 2018 19:48
Comment thread src/Common/src/System/Net/SafeCloseSocket.Unix.cs
@wfurt

wfurt commented Oct 1, 2018

Copy link
Copy Markdown
Member Author

@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

karelz commented Dec 19, 2018

Copy link
Copy Markdown
Member

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