Skip to content

clean up some logic around non-blocking sockets on Unix - #50985

Merged
geoffkizer merged 1 commit into
dotnet:mainfrom
geoffkizer:cleanupisnonblocking
Apr 15, 2021
Merged

clean up some logic around non-blocking sockets on Unix#50985
geoffkizer merged 1 commit into
dotnet:mainfrom
geoffkizer:cleanupisnonblocking

Conversation

@geoffkizer

Copy link
Copy Markdown
Contributor

Mostly comments and renames for clarity, but also remove the _underlyingHandleNonBlocking field on SafeSocketHandle since it is duplicative.

@tmds @benadams

@ghost

ghost commented Apr 9, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Mostly comments and renames for clarity, but also remove the _underlyingHandleNonBlocking field on SafeSocketHandle since it is duplicative.

@tmds @benadams

Author: geoffkizer
Assignees: -
Labels:

area-System.Net.Sockets

Milestone: -
@geoffkizer

Copy link
Copy Markdown
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).
}

_nonBlockingSet = true;
_isHandleNonBlocking = true;

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.

Unrelated to the PR. I wonder if this may get reordered so the field is assigned before the socket becomes non-blocking? Or maybe the PInvoke/syscall implies this is ordered?
If it were reordered, for a concurrent caller the function may return before the socket is actually non-blocking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure (but not 100%) that this syscall will prevent reordering here.

@tmds tmds left a comment

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.

LGTM!

@geoffkizer

Copy link
Copy Markdown
Contributor Author

@dotnet/ncl can someone review and approve?

@antonfirsov antonfirsov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

@geoffkizer
geoffkizer merged commit 2aa0348 into dotnet:main Apr 15, 2021
@geoffkizer
geoffkizer deleted the cleanupisnonblocking branch April 15, 2021 19:57
@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.