Elide SocketFlags in Socket API - #57634

Merged
antonfirsov merged 5 commits into
dotnet:mainfrom
i3arnon:socket-flags
Sep 14, 2021
Merged

Elide SocketFlags in Socket API#57634
antonfirsov merged 5 commits into
dotnet:mainfrom
i3arnon:socket-flags

Conversation

@i3arnon

@i3arnoni3arnon commented Aug 18, 2021

Copy link
Copy Markdown
Contributor

@ghost

ghost commented Aug 18, 2021

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghostghost added the community-contribution Indicates that the PR has been added by a community member label Aug 18, 2021
@ghost

ghost commented Aug 18, 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

Fix #43934

Author:i3arnon
Assignees:-
Labels:

area-System.Net.Sockets, new-api-needs-documentation

Milestone:-

@i3arnon

i3arnon commented Aug 18, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Line 1810 in 69de57b

flags=operation.ReceivedFlags;

antonfirsov
antonfirsov reviewed Aug 18, 2021
View reviewed changes

@antonfirsovantonfirsov 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.

Thank you for the contribution! The implementation looks good.

What's worrying me that the overloads that elide SocketFlags are missing test coverage (both old and new ones). I think we should fix this as part of the current PR.

I believe it would be an overkill to address this by duplicating our tests a couple of more times by adding more SocketHelperBase subclasses, unnecessarily penalizing our test execution time. My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?

I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:

@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)

Comment threadsrc/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
@i3arnon

i3arnon commented Aug 19, 2021

Copy link
Copy Markdown
ContributorAuthor

My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket

Done.

@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)

I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.

@geoffkizer

geoffkizer commented Aug 19, 2021

Copy link
Copy Markdown
Contributor

My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?

Just to be clear -- the idea here is that we cover these new overloads by making some of the existing test variations use them, but leave some using the old overloads so we continue to have coverage for those as well?

That seems reasonable to me.

Let's make sure to add comments that explain this, otherwise it will be far from obvious and we'll look at the code in the future and say "why are we using these overloads here but those overloads there??"

@geoffkizer

geoffkizer commented Aug 19, 2021

Copy link
Copy Markdown
Contributor

I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.

This is a strong argument for not eliding the flags on ReceiveMessageFrom.

@karelzkarelz added this to the 7.0.0 milestone Aug 20, 2021
antonfirsov
antonfirsov reviewed Aug 27, 2021
View reviewed changes

@antonfirsovantonfirsov 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.

Just one thing, and this is good to merge. I think we should leave out the sync ReceiveMessageFrom.

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs Outdated
@antonfirsov

antonfirsov commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Sep 14, 2021

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

antonfirsov
antonfirsov approved these changes Sep 14, 2021
View reviewed changes

@antonfirsovantonfirsov 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. Shall be good to merge once outerloop CI completes.

@antonfirsov

antonfirsov commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

Outerloop failures are unrelated (#32955 and a bunch of ProcessStartInfo tests on Windows 11).

@antonfirsov
antonfirsov merged commit 975aa54 into dotnet:mainSep 14, 2021
@antonfirsov

antonfirsov commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

@i3arnon thanks again for the work and your patience!

@i3arnon
i3arnon deleted the socket-flags branch September 14, 2021 19:39
@CarnaViireCarnaViire mentioned this pull request Oct 8, 2021
Open
@ghostghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonfirsovantonfirsov

@geoffkizergeoffkizer

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Consider adding Socket Send/ReceiveAsync overloads that elide SocketFlags argument

4 participants

@i3arnon@geoffkizer@antonfirsov@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Elide SocketFlags in Socket API - #57634
Merged
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Sep 14, 2021
Merged
Elide SocketFlags in Socket API#57634
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Conversation
@i3arnon
@i3arnoni3arnon
commented
Aug 18, 2021

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
Aug 18, 2021

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghostghost
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 18, 2021
@ghost
ghost
commented
Aug 18, 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
Fix #43934
Author:i3arnon
Assignees:-
Labels:
area-System.Net.Sockets, new-api-needs-documentation
Milestone:-
@i3arnon
i3arnon
commented
Aug 18, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Line 1810
in
69de57b
flags=operation.ReceivedFlags;
antonfirsov
antonfirsov
reviewed
Aug 18, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Thank you for the contribution! The implementation looks good.
What's worrying me that the overloads that elide SocketFlags are missing test coverage (both old and new ones). I think we should fix this as part of the current PR.
I believe it would be an overkill to address this by duplicating our tests a couple of more times by adding more SocketHelperBase subclasses, unnecessarily penalizing our test execution time. My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
Comment threadsrc/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
@i3arnon
i3arnon
commented
Aug 19, 2021

Copy link


Copy Markdown

ContributorAuthor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket
Done.
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
Just to be clear -- the idea here is that we cover these new overloads by making some of the existing test variations use them, but leave some using the old overloads so we continue to have coverage for those as well?
That seems reasonable to me.
Let's make sure to add comments that explain this, otherwise it will be far from obvious and we'll look at the code in the future and say "why are we using these overloads here but those overloads there??"
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
This is a strong argument for not eliding the flags on ReceiveMessageFrom.
@karelzkarelz
added this to the 7.0.0 milestone
Aug 20, 2021
antonfirsov
antonfirsov
reviewed
Aug 27, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Just one thing, and this is good to merge. I think we should leave out the sync ReceiveMessageFrom.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs
Outdated

@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 14, 2021

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Sep 14, 2021
View reviewed changes
@antonfirsovantonfirsov
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. Shall be good to merge once outerloop CI completes.
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
Outerloop failures are unrelated (#32955 and a bunch of ProcessStartInfo tests on Windows 11).
@antonfirsov
antonfirsov
merged commit 975aa54
into
dotnet:mainSep 14, 2021
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
@i3arnon thanks again for the work and your patience!
@i3arnon
i3arnon
deleted the

socket-flags

branch
September 14, 2021 19:39
@CarnaViireCarnaViire
mentioned this pull request
Oct 8, 2021

Open

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonfirsovantonfirsov
@geoffkizergeoffkizer

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Consider adding Socket Send/ReceiveAsync overloads that elide SocketFlags argument


4 participants

@i3arnon@geoffkizer@antonfirsov@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Elide SocketFlags in Socket API - #57634
Merged
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Sep 14, 2021
Merged
Elide SocketFlags in Socket API#57634
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Conversation
@i3arnon
@i3arnoni3arnon
commented
Aug 18, 2021

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
Aug 18, 2021

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghostghost
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 18, 2021
@ghost
ghost
commented
Aug 18, 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
Fix #43934
Author:i3arnon
Assignees:-
Labels:
area-System.Net.Sockets, new-api-needs-documentation
Milestone:-
@i3arnon
i3arnon
commented
Aug 18, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Line 1810
in
69de57b
flags=operation.ReceivedFlags;
antonfirsov
antonfirsov
reviewed
Aug 18, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Thank you for the contribution! The implementation looks good.
What's worrying me that the overloads that elide SocketFlags are missing test coverage (both old and new ones). I think we should fix this as part of the current PR.
I believe it would be an overkill to address this by duplicating our tests a couple of more times by adding more SocketHelperBase subclasses, unnecessarily penalizing our test execution time. My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
Comment threadsrc/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
@i3arnon
i3arnon
commented
Aug 19, 2021

Copy link


Copy Markdown

ContributorAuthor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket
Done.
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
Just to be clear -- the idea here is that we cover these new overloads by making some of the existing test variations use them, but leave some using the old overloads so we continue to have coverage for those as well?
That seems reasonable to me.
Let's make sure to add comments that explain this, otherwise it will be far from obvious and we'll look at the code in the future and say "why are we using these overloads here but those overloads there??"
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
This is a strong argument for not eliding the flags on ReceiveMessageFrom.
@karelzkarelz
added this to the 7.0.0 milestone
Aug 20, 2021
antonfirsov
antonfirsov
reviewed
Aug 27, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Just one thing, and this is good to merge. I think we should leave out the sync ReceiveMessageFrom.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs
Outdated

@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 14, 2021

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Sep 14, 2021
View reviewed changes
@antonfirsovantonfirsov
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. Shall be good to merge once outerloop CI completes.
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
Outerloop failures are unrelated (#32955 and a bunch of ProcessStartInfo tests on Windows 11).
@antonfirsov
antonfirsov
merged commit 975aa54
into
dotnet:mainSep 14, 2021
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
@i3arnon thanks again for the work and your patience!
@i3arnon
i3arnon
deleted the

socket-flags

branch
September 14, 2021 19:39
@CarnaViireCarnaViire
mentioned this pull request
Oct 8, 2021

Open

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonfirsovantonfirsov
@geoffkizergeoffkizer

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Consider adding Socket Send/ReceiveAsync overloads that elide SocketFlags argument


4 participants

@i3arnon@geoffkizer@antonfirsov@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Elide SocketFlags in Socket API - #57634
Merged
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Sep 14, 2021
Merged
Elide SocketFlags in Socket API#57634
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Conversation
@i3arnon
@i3arnoni3arnon
commented
Aug 18, 2021

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
Aug 18, 2021

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghostghost
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 18, 2021
@ghost
ghost
commented
Aug 18, 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
Fix #43934
Author:i3arnon
Assignees:-
Labels:
area-System.Net.Sockets, new-api-needs-documentation
Milestone:-
@i3arnon
i3arnon
commented
Aug 18, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Line 1810
in
69de57b
flags=operation.ReceivedFlags;
antonfirsov
antonfirsov
reviewed
Aug 18, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Thank you for the contribution! The implementation looks good.
What's worrying me that the overloads that elide SocketFlags are missing test coverage (both old and new ones). I think we should fix this as part of the current PR.
I believe it would be an overkill to address this by duplicating our tests a couple of more times by adding more SocketHelperBase subclasses, unnecessarily penalizing our test execution time. My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
Comment threadsrc/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
@i3arnon
i3arnon
commented
Aug 19, 2021

Copy link


Copy Markdown

ContributorAuthor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket
Done.
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
Just to be clear -- the idea here is that we cover these new overloads by making some of the existing test variations use them, but leave some using the old overloads so we continue to have coverage for those as well?
That seems reasonable to me.
Let's make sure to add comments that explain this, otherwise it will be far from obvious and we'll look at the code in the future and say "why are we using these overloads here but those overloads there??"
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
This is a strong argument for not eliding the flags on ReceiveMessageFrom.
@karelzkarelz
added this to the 7.0.0 milestone
Aug 20, 2021
antonfirsov
antonfirsov
reviewed
Aug 27, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Just one thing, and this is good to merge. I think we should leave out the sync ReceiveMessageFrom.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs
Outdated

@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 14, 2021

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Sep 14, 2021
View reviewed changes
@antonfirsovantonfirsov
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. Shall be good to merge once outerloop CI completes.
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
Outerloop failures are unrelated (#32955 and a bunch of ProcessStartInfo tests on Windows 11).
@antonfirsov
antonfirsov
merged commit 975aa54
into
dotnet:mainSep 14, 2021
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
@i3arnon thanks again for the work and your patience!
@i3arnon
i3arnon
deleted the

socket-flags

branch
September 14, 2021 19:39
@CarnaViireCarnaViire
mentioned this pull request
Oct 8, 2021

Open

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonfirsovantonfirsov
@geoffkizergeoffkizer

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Consider adding Socket Send/ReceiveAsync overloads that elide SocketFlags argument


4 participants

@i3arnon@geoffkizer@antonfirsov@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Elide SocketFlags in Socket API - #57634
Merged
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Sep 14, 2021
Merged
Elide SocketFlags in Socket API#57634
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Conversation
@i3arnon
@i3arnoni3arnon
commented
Aug 18, 2021

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
Aug 18, 2021

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghostghost
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 18, 2021
@ghost
ghost
commented
Aug 18, 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
Fix #43934
Author:i3arnon
Assignees:-
Labels:
area-System.Net.Sockets, new-api-needs-documentation
Milestone:-
@i3arnon
i3arnon
commented
Aug 18, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Line 1810
in
69de57b
flags=operation.ReceivedFlags;
antonfirsov
antonfirsov
reviewed
Aug 18, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Thank you for the contribution! The implementation looks good.
What's worrying me that the overloads that elide SocketFlags are missing test coverage (both old and new ones). I think we should fix this as part of the current PR.
I believe it would be an overkill to address this by duplicating our tests a couple of more times by adding more SocketHelperBase subclasses, unnecessarily penalizing our test execution time. My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
Comment threadsrc/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
@i3arnon
i3arnon
commented
Aug 19, 2021

Copy link


Copy Markdown

ContributorAuthor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket
Done.
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
Just to be clear -- the idea here is that we cover these new overloads by making some of the existing test variations use them, but leave some using the old overloads so we continue to have coverage for those as well?
That seems reasonable to me.
Let's make sure to add comments that explain this, otherwise it will be far from obvious and we'll look at the code in the future and say "why are we using these overloads here but those overloads there??"
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
This is a strong argument for not eliding the flags on ReceiveMessageFrom.
@karelzkarelz
added this to the 7.0.0 milestone
Aug 20, 2021
antonfirsov
antonfirsov
reviewed
Aug 27, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Just one thing, and this is good to merge. I think we should leave out the sync ReceiveMessageFrom.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs
Outdated

@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 14, 2021

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Sep 14, 2021
View reviewed changes
@antonfirsovantonfirsov
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. Shall be good to merge once outerloop CI completes.
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
Outerloop failures are unrelated (#32955 and a bunch of ProcessStartInfo tests on Windows 11).
@antonfirsov
antonfirsov
merged commit 975aa54
into
dotnet:mainSep 14, 2021
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
@i3arnon thanks again for the work and your patience!
@i3arnon
i3arnon
deleted the

socket-flags

branch
September 14, 2021 19:39
@CarnaViireCarnaViire
mentioned this pull request
Oct 8, 2021

Open

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonfirsovantonfirsov
@geoffkizergeoffkizer

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Consider adding Socket Send/ReceiveAsync overloads that elide SocketFlags argument


4 participants

@i3arnon@geoffkizer@antonfirsov@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Elide SocketFlags in Socket API - #57634
Merged
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Sep 14, 2021
Merged
Elide SocketFlags in Socket API#57634
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Conversation
@i3arnon
@i3arnoni3arnon
commented
Aug 18, 2021

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
Aug 18, 2021

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghostghost
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 18, 2021
@ghost
ghost
commented
Aug 18, 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
Fix #43934
Author:i3arnon
Assignees:-
Labels:
area-System.Net.Sockets, new-api-needs-documentation
Milestone:-
@i3arnon
i3arnon
commented
Aug 18, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Line 1810
in
69de57b
flags=operation.ReceivedFlags;
antonfirsov
antonfirsov
reviewed
Aug 18, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Thank you for the contribution! The implementation looks good.
What's worrying me that the overloads that elide SocketFlags are missing test coverage (both old and new ones). I think we should fix this as part of the current PR.
I believe it would be an overkill to address this by duplicating our tests a couple of more times by adding more SocketHelperBase subclasses, unnecessarily penalizing our test execution time. My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
Comment threadsrc/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
@i3arnon
i3arnon
commented
Aug 19, 2021

Copy link


Copy Markdown

ContributorAuthor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket
Done.
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
Just to be clear -- the idea here is that we cover these new overloads by making some of the existing test variations use them, but leave some using the old overloads so we continue to have coverage for those as well?
That seems reasonable to me.
Let's make sure to add comments that explain this, otherwise it will be far from obvious and we'll look at the code in the future and say "why are we using these overloads here but those overloads there??"
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
This is a strong argument for not eliding the flags on ReceiveMessageFrom.
@karelzkarelz
added this to the 7.0.0 milestone
Aug 20, 2021
antonfirsov
antonfirsov
reviewed
Aug 27, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Just one thing, and this is good to merge. I think we should leave out the sync ReceiveMessageFrom.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs
Outdated

@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 14, 2021

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Sep 14, 2021
View reviewed changes
@antonfirsovantonfirsov
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. Shall be good to merge once outerloop CI completes.
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
Outerloop failures are unrelated (#32955 and a bunch of ProcessStartInfo tests on Windows 11).
@antonfirsov
antonfirsov
merged commit 975aa54
into
dotnet:mainSep 14, 2021
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
@i3arnon thanks again for the work and your patience!
@i3arnon
i3arnon
deleted the

socket-flags

branch
September 14, 2021 19:39
@CarnaViireCarnaViire
mentioned this pull request
Oct 8, 2021

Open

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonfirsovantonfirsov
@geoffkizergeoffkizer

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Consider adding Socket Send/ReceiveAsync overloads that elide SocketFlags argument


4 participants

@i3arnon@geoffkizer@antonfirsov@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Elide SocketFlags in Socket API - #57634
Merged
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Sep 14, 2021
Merged
Elide SocketFlags in Socket API#57634
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Conversation
@i3arnon
@i3arnoni3arnon
commented
Aug 18, 2021

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
Aug 18, 2021

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghostghost
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 18, 2021
@ghost
ghost
commented
Aug 18, 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
Fix #43934
Author:i3arnon
Assignees:-
Labels:
area-System.Net.Sockets, new-api-needs-documentation
Milestone:-
@i3arnon
i3arnon
commented
Aug 18, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Line 1810
in
69de57b
flags=operation.ReceivedFlags;
antonfirsov
antonfirsov
reviewed
Aug 18, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Thank you for the contribution! The implementation looks good.
What's worrying me that the overloads that elide SocketFlags are missing test coverage (both old and new ones). I think we should fix this as part of the current PR.
I believe it would be an overkill to address this by duplicating our tests a couple of more times by adding more SocketHelperBase subclasses, unnecessarily penalizing our test execution time. My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
Comment threadsrc/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
@i3arnon
i3arnon
commented
Aug 19, 2021

Copy link


Copy Markdown

ContributorAuthor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket
Done.
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
Just to be clear -- the idea here is that we cover these new overloads by making some of the existing test variations use them, but leave some using the old overloads so we continue to have coverage for those as well?
That seems reasonable to me.
Let's make sure to add comments that explain this, otherwise it will be far from obvious and we'll look at the code in the future and say "why are we using these overloads here but those overloads there??"
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
This is a strong argument for not eliding the flags on ReceiveMessageFrom.
@karelzkarelz
added this to the 7.0.0 milestone
Aug 20, 2021
antonfirsov
antonfirsov
reviewed
Aug 27, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Just one thing, and this is good to merge. I think we should leave out the sync ReceiveMessageFrom.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs
Outdated

@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 14, 2021

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Sep 14, 2021
View reviewed changes
@antonfirsovantonfirsov
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. Shall be good to merge once outerloop CI completes.
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
Outerloop failures are unrelated (#32955 and a bunch of ProcessStartInfo tests on Windows 11).
@antonfirsov
antonfirsov
merged commit 975aa54
into
dotnet:mainSep 14, 2021
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
@i3arnon thanks again for the work and your patience!
@i3arnon
i3arnon
deleted the

socket-flags

branch
September 14, 2021 19:39
@CarnaViireCarnaViire
mentioned this pull request
Oct 8, 2021

Open

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonfirsovantonfirsov
@geoffkizergeoffkizer

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Consider adding Socket Send/ReceiveAsync overloads that elide SocketFlags argument


4 participants

@i3arnon@geoffkizer@antonfirsov@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Elide SocketFlags in Socket API - #57634
Merged
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Sep 14, 2021
Merged
Elide SocketFlags in Socket API#57634
antonfirsov merged 5 commits into
dotnet:mainfrom 
i3arnon:socket-flags
Conversation
@i3arnon
@i3arnoni3arnon
commented
Aug 18, 2021

Copy link


Copy Markdown

Contributor
@ghost
ghost
commented
Aug 18, 2021

Copy link


Copy Markdown

Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
@ghostghost
added
the 
community-contribution
Indicates that the PR has been added by a community member
label
Aug 18, 2021
@ghost
ghost
commented
Aug 18, 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
Fix #43934
Author:i3arnon
Assignees:-
Labels:
area-System.Net.Sockets, new-api-needs-documentation
Milestone:-
@i3arnon
i3arnon
commented
Aug 18, 2021•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Line 1810
in
69de57b
flags=operation.ReceivedFlags;
antonfirsov
antonfirsov
reviewed
Aug 18, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Thank you for the contribution! The implementation looks good.
What's worrying me that the overloads that elide SocketFlags are missing test coverage (both old and new ones). I think we should fix this as part of the current PR.
I believe it would be an overkill to address this by duplicating our tests a couple of more times by adding more SocketHelperBase subclasses, unnecessarily penalizing our test execution time. My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
I've left out ReceiveMessageFrom as the SocketFlags does return data from the operation:
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
Comment threadsrc/libraries/System.Net.Sockets/ref/System.Net.Sockets.cs
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
@i3arnon
i3arnon
commented
Aug 19, 2021

Copy link


Copy Markdown

ContributorAuthor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket
Done.
@geoffkizer do you think it's a useful overload regardless? The approved proposal is also questioning them: #43934 (comment)
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
My recommendation would be to change the existing SocketHelperTask and SocketHelperSpanSync to elide SocketFlags in their calls to Socket. We aren't really testing the effect of SocketFlags in those tests anyways. @geoffkizer do you agree?
Just to be clear -- the idea here is that we cover these new overloads by making some of the existing test variations use them, but leave some using the old overloads so we continue to have coverage for those as well?
That seems reasonable to me.
Let's make sure to add comments that explain this, otherwise it will be far from obvious and we'll look at the code in the future and say "why are we using these overloads here but those overloads there??"
@geoffkizer
geoffkizer
commented
Aug 19, 2021

Copy link


Copy Markdown

Contributor
I'll add that also in the sync overloads, there's an overload for ReceiveFrom that elides the flags, which are not passed by ref, but there isn't one for ReceiveMessageFrom which passes the flags by ref.
This is a strong argument for not eliding the flags on ReceiveMessageFrom.
@karelzkarelz
added this to the 7.0.0 milestone
Aug 20, 2021
antonfirsov
antonfirsov
reviewed
Aug 27, 2021
View reviewed changes
@antonfirsovantonfirsov
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.

Just one thing, and this is good to merge. I think we should leave out the sync ReceiveMessageFrom.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SocketTestHelper.cs
Outdated

@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
/azp run runtime-libraries-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Sep 14, 2021

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Sep 14, 2021
View reviewed changes
@antonfirsovantonfirsov
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. Shall be good to merge once outerloop CI completes.
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
Outerloop failures are unrelated (#32955 and a bunch of ProcessStartInfo tests on Windows 11).
@antonfirsov
antonfirsov
merged commit 975aa54
into
dotnet:mainSep 14, 2021
@antonfirsov
antonfirsov
commented
Sep 14, 2021

Copy link


Copy Markdown

Contributor
@i3arnon thanks again for the work and your patience!
@i3arnon
i3arnon
deleted the

socket-flags

branch
September 14, 2021 19:39
@CarnaViireCarnaViire
mentioned this pull request
Oct 8, 2021

Open

@ghostghost
locked as resolved and limited conversation to collaborators
Nov 3, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers


1 more reviewer

@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonfirsovantonfirsov
@geoffkizergeoffkizer

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects


None yet


Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Consider adding Socket Send/ReceiveAsync overloads that elide SocketFlags argument


4 participants

@i3arnon@geoffkizer@antonfirsov@karelz