Skip to content

Unified to throw NotSupportedException when SendFile() for connectionless sockets - #87108

Merged
MihaZupan merged 5 commits into
dotnet:mainfrom
rhirano0715:issue47472
Aug 9, 2023
Merged

Unified to throw NotSupportedException when SendFile() for connectionless sockets#87108
MihaZupan merged 5 commits into
dotnet:mainfrom
rhirano0715:issue47472

Conversation

@rhirano0715

@rhirano0715rhirano0715 commented Jun 4, 2023

Copy link
Copy Markdown
Contributor

The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.

With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.

Fix#47472

@rhirano0715
Unified to throw NotSupportedException when SendFile() for connection…
cb8c786
…less sockets
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP.
The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@ghostghost added area-System.Net.Sockets community-contribution Indicates that the PR has been added by a community member labels Jun 4, 2023
@ghost

ghost commented Jun 4, 2023

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

The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.

With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.

Fix #47472

Author:rhirano0715
Assignees:-
Labels:

area-System.Net.Sockets

Milestone:-

@rhirano0715

rhirano0715 commented Jun 4, 2023

Copy link
Copy Markdown
ContributorAuthor

@dotnet-policy-service agree

@build-analysisbuild-analysisBot mentioned this pull request Jun 4, 2023
Closed
@karelz
karelz requested a review from antonfirsovJune 27, 2023 13:18
@karelz

karelz commented Jul 3, 2023

Copy link
Copy Markdown
Member

@antonfirsov ping, can you please take a look?

antonfirsov
antonfirsov reviewed Jul 4, 2023
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.

Sorry for the long delay!

Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs Outdated
Comment on lines +2341 to +2344
if (_protocolType == ProtocolType.Udp)
{
throw new NotSupportedException(SR.net_notconnected);
}

@antonfirsovantonfirsovJul 3, 2023

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.

Given that BeginSendFile calls into SendFileAsync, this check is redundant.

Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
if (usePreAndPostbufferOverload)
{
ex = await Assert.ThrowsAsync<SocketException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
await Assert.ThrowsAsync<NotSupportedException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));

@antonfirsovantonfirsovJul 3, 2023

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.

This test case should not be [PlatformSpecific(TestPlatforms.Windows)] anymore.

Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs Outdated
Comment on lines +673 to +676
if (_protocolType == ProtocolType.Udp)
{
throw new NotSupportedException(SR.net_notconnected);
}

@antonfirsovantonfirsovJul 4, 2023•
edited
Loading

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.

Doesn't the check if (!IsConnectionOriented) do the job already? In #47472 I only tested the sync overload.

Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs Outdated
Comment on lines +1257 to +1260
if (_protocolType == ProtocolType.Udp)
{
throw new NotSupportedException(SR.net_notconnected);
}

@antonfirsovantonfirsovJul 4, 2023

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.

The async variant checks for if (!IsConnectionOriented) while the sync version checks (!Connected). I think that - instead of limiting the error case explicitly to UDP - we should consolidate these checks, throwing in case of (!IsConnectionOriented || !Connected).

@karelz

karelz commented Jul 18, 2023

Copy link
Copy Markdown
Member

@rhirano0715 will you have time to look at the CR feedback from @antonfirsov please?

rhirano0715and others added 3 commits July 19, 2023 18:06
@rhirano0715
Merge branch 'dotnet:main' into issue47472
aaf4917
@rhirano0715
Change to throws NotSupportedException if !IsConnectionOriented o…
a171986
…r `!Connected`.
Before:.
Throws `NotSupportedException` on UDP.
After:
Throws `NotSupportedException` if `!IsConnectionOriented` or `!Connected`.
@rhirano0715
Changed test case UdpConnection_ThrowsException to run regardless o…
7bd5d7c
…f platform.
This was referenced Jul 19, 2023
Closed
Closed
@rhirano0715

rhirano0715 commented Jul 19, 2023

Copy link
Copy Markdown
ContributorAuthor

@karelz@antonfirsov
I'm sorry for not being able to respond.
Thank you for your review.
I have committed the correction.

karelz
karelz reviewed Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs Outdated
@rhirano0715@karelz
Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket…
3954c40
….Tasks.cs
Co-authored-by: Karel Zikmund <karelz@microsoft.com>
@antonfirsovantonfirsov added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Aug 4, 2023
@antonfirsov

antonfirsov commented Aug 4, 2023

Copy link
Copy Markdown
Contributor

/azp run runtime

@antonfirsov

antonfirsov commented Aug 4, 2023

Copy link
Copy Markdown
Contributor

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Aug 4, 2023

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

1 similar comment
@azure-pipelines

azure-pipelinesBot commented Aug 4, 2023

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

antonfirsov
antonfirsov approved these changes Aug 4, 2023
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, thanks, and sorry for the slow response. Started some extra CI runs just in case.

This was referenced Aug 4, 2023
Closed
Closed
Closed
Closed
Closed
This was referenced Aug 4, 2023
Closed
Closed
@MihaZupanMihaZupan reopened this Aug 8, 2023
@MihaZupan

MihaZupan commented Aug 8, 2023

Copy link
Copy Markdown
Member

/azp run runtime-extra-platforms

@azure-pipelines

azure-pipelinesBot commented Aug 8, 2023

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

@build-analysisbuild-analysisBot mentioned this pull request Aug 8, 2023
Closed
@MihaZupan
MihaZupan merged commit 24f1513 into dotnet:mainAug 9, 2023
@karelzkarelz added this to the 8.0.0 milestone Aug 10, 2023
@ghostghost locked as resolved and limited conversation to collaborators Sep 9, 2023
@antonfirsovantonfirsov removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 17, 2023
@rhirano0715
rhirano0715 deleted the issue47472 branch December 6, 2023 08:21
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@karelzkarelzkarelz left review comments
+1 more reviewer
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@antonfirsovantonfirsov

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Socket.SendFile() should fail with a consistent exception for connectionless sockets

4 participants

@rhirano0715@karelz@antonfirsov@MihaZupan
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Unified to throw NotSupportedException when SendFile() for connectionless sockets by rhirano0715 · Pull Request #87108 · dotnet/runtime · GitHub
Skip to content
Unified to throw NotSupportedException when SendFile() for connectionless sockets - #87108
Merged
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Aug 9, 2023
Merged
Unified to throw NotSupportedException when SendFile() for connectionless sockets#87108
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Conversation
@rhirano0715
@rhirano0715rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

Contributor
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@rhirano0715
Unified to throw NotSupportedException when SendFile() for connection…
cb8c786
…less sockets
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP.
The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@ghostghost
added

area-System.Net.Sockets

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 4, 2023
@ghost
ghost
commented
Jun 4, 2023

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
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix #47472
Author:rhirano0715
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@rhirano0715
rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet-policy-service agree
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 4, 2023

Closed

@karelz
karelz
requested a review
from antonfirsovJune 27, 2023 13:18
@karelz
karelz
commented
Jul 3, 2023

Copy link


Copy Markdown

Member
@antonfirsov ping, can you please take a look?
antonfirsov
antonfirsov
reviewed
Jul 4, 2023
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.

Sorry for the long delay!
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+2341
 to 
+2344

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 3, 2023

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.

Given that BeginSendFile calls into SendFileAsync, this check is redundant.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
 if (usePreAndPostbufferOverload)
 {
ex = await Assert.ThrowsAsync<SocketException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
 await Assert.ThrowsAsync<NotSupportedException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
@antonfirsovantonfirsovJul 3, 2023

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.

This test case should not be [PlatformSpecific(TestPlatforms.Windows)] anymore.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated


Comment on lines

+673
 to 
+676

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023•

edited

Loading

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.

Doesn't the check if (!IsConnectionOriented) do the job already? In #47472 I only tested the sync overload.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+1257
 to 
+1260

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023

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.

The async variant checks for if (!IsConnectionOriented) while the sync version checks (!Connected). I think that - instead of limiting the error case explicitly to UDP - we should consolidate these checks, throwing in case of (!IsConnectionOriented || !Connected).
@karelz
karelz
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@rhirano0715 will you have time to look at the CR feedback from @antonfirsov please?
rhirano0715and others
added 3 commits
July 19, 2023 18:06
@rhirano0715
Merge branch 'dotnet:main' into issue47472
aaf4917
@rhirano0715
Change to throws NotSupportedException if !IsConnectionOriented o…
a171986
…r `!Connected`.
Before:.
Throws `NotSupportedException` on UDP.
After:
Throws `NotSupportedException` if `!IsConnectionOriented` or `!Connected`.
@rhirano0715
Changed test case UdpConnection_ThrowsException to run regardless o…
7bd5d7c
…f platform.

This was referenced Jul 19, 2023

Closed


Closed

@rhirano0715
rhirano0715
commented
Jul 19, 2023

Copy link


Copy Markdown

ContributorAuthor
@karelz@antonfirsov

I'm sorry for not being able to respond.

Thank you for your review.

I have committed the correction.
karelz
karelz
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated

@rhirano0715@karelz
Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket…
3954c40
….Tasks.cs
Co-authored-by: Karel Zikmund <karelz@microsoft.com>
@antonfirsovantonfirsov
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Aug 4, 2023
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Aug 4, 2023
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, thanks, and sorry for the slow response. Started some extra CI runs just in case.

This was referenced Aug 4, 2023

Closed


Closed


Closed


Closed


Closed


This was referenced Aug 4, 2023

Closed


Closed

@MihaZupanMihaZupan
reopened this
Aug 8, 2023
@MihaZupan
MihaZupan
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 8, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 8, 2023

Closed

@MihaZupan
MihaZupan
merged commit 24f1513
into
dotnet:mainAug 9, 2023
@karelzkarelz
added this to the 8.0.0 milestone
Aug 10, 2023
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 9, 2023
@antonfirsovantonfirsov
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 17, 2023
@rhirano0715
rhirano0715
deleted the

issue47472

branch
December 6, 2023 08:21
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments

+1 more reviewer

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

Assignees

@antonfirsovantonfirsov

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Socket.SendFile() should fail with a consistent exception for connectionless sockets


4 participants

@rhirano0715@karelz@antonfirsov@MihaZupan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Unified to throw NotSupportedException when SendFile() for connectionless sockets by rhirano0715 · Pull Request #87108 · dotnet/runtime · GitHub
Skip to content
Unified to throw NotSupportedException when SendFile() for connectionless sockets - #87108
Merged
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Aug 9, 2023
Merged
Unified to throw NotSupportedException when SendFile() for connectionless sockets#87108
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Conversation
@rhirano0715
@rhirano0715rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

Contributor
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@rhirano0715
Unified to throw NotSupportedException when SendFile() for connection…
cb8c786
…less sockets
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP.
The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@ghostghost
added

area-System.Net.Sockets

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 4, 2023
@ghost
ghost
commented
Jun 4, 2023

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
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix #47472
Author:rhirano0715
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@rhirano0715
rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet-policy-service agree
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 4, 2023

Closed

@karelz
karelz
requested a review
from antonfirsovJune 27, 2023 13:18
@karelz
karelz
commented
Jul 3, 2023

Copy link


Copy Markdown

Member
@antonfirsov ping, can you please take a look?
antonfirsov
antonfirsov
reviewed
Jul 4, 2023
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.

Sorry for the long delay!
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+2341
 to 
+2344

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 3, 2023

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.

Given that BeginSendFile calls into SendFileAsync, this check is redundant.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
 if (usePreAndPostbufferOverload)
 {
ex = await Assert.ThrowsAsync<SocketException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
 await Assert.ThrowsAsync<NotSupportedException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
@antonfirsovantonfirsovJul 3, 2023

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.

This test case should not be [PlatformSpecific(TestPlatforms.Windows)] anymore.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated


Comment on lines

+673
 to 
+676

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023•

edited

Loading

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.

Doesn't the check if (!IsConnectionOriented) do the job already? In #47472 I only tested the sync overload.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+1257
 to 
+1260

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023

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.

The async variant checks for if (!IsConnectionOriented) while the sync version checks (!Connected). I think that - instead of limiting the error case explicitly to UDP - we should consolidate these checks, throwing in case of (!IsConnectionOriented || !Connected).
@karelz
karelz
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@rhirano0715 will you have time to look at the CR feedback from @antonfirsov please?
rhirano0715and others
added 3 commits
July 19, 2023 18:06
@rhirano0715
Merge branch 'dotnet:main' into issue47472
aaf4917
@rhirano0715
Change to throws NotSupportedException if !IsConnectionOriented o…
a171986
…r `!Connected`.
Before:.
Throws `NotSupportedException` on UDP.
After:
Throws `NotSupportedException` if `!IsConnectionOriented` or `!Connected`.
@rhirano0715
Changed test case UdpConnection_ThrowsException to run regardless o…
7bd5d7c
…f platform.

This was referenced Jul 19, 2023

Closed


Closed

@rhirano0715
rhirano0715
commented
Jul 19, 2023

Copy link


Copy Markdown

ContributorAuthor
@karelz@antonfirsov

I'm sorry for not being able to respond.

Thank you for your review.

I have committed the correction.
karelz
karelz
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated

@rhirano0715@karelz
Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket…
3954c40
….Tasks.cs
Co-authored-by: Karel Zikmund <karelz@microsoft.com>
@antonfirsovantonfirsov
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Aug 4, 2023
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Aug 4, 2023
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, thanks, and sorry for the slow response. Started some extra CI runs just in case.

This was referenced Aug 4, 2023

Closed


Closed


Closed


Closed


Closed


This was referenced Aug 4, 2023

Closed


Closed

@MihaZupanMihaZupan
reopened this
Aug 8, 2023
@MihaZupan
MihaZupan
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 8, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 8, 2023

Closed

@MihaZupan
MihaZupan
merged commit 24f1513
into
dotnet:mainAug 9, 2023
@karelzkarelz
added this to the 8.0.0 milestone
Aug 10, 2023
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 9, 2023
@antonfirsovantonfirsov
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 17, 2023
@rhirano0715
rhirano0715
deleted the

issue47472

branch
December 6, 2023 08:21
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments

+1 more reviewer

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

Assignees

@antonfirsovantonfirsov

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Socket.SendFile() should fail with a consistent exception for connectionless sockets


4 participants

@rhirano0715@karelz@antonfirsov@MihaZupan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Unified to throw NotSupportedException when SendFile() for connectionless sockets by rhirano0715 · Pull Request #87108 · dotnet/runtime · GitHub
Skip to content
Unified to throw NotSupportedException when SendFile() for connectionless sockets - #87108
Merged
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Aug 9, 2023
Merged
Unified to throw NotSupportedException when SendFile() for connectionless sockets#87108
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Conversation
@rhirano0715
@rhirano0715rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

Contributor
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@rhirano0715
Unified to throw NotSupportedException when SendFile() for connection…
cb8c786
…less sockets
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP.
The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@ghostghost
added

area-System.Net.Sockets

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 4, 2023
@ghost
ghost
commented
Jun 4, 2023

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
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix #47472
Author:rhirano0715
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@rhirano0715
rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet-policy-service agree
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 4, 2023

Closed

@karelz
karelz
requested a review
from antonfirsovJune 27, 2023 13:18
@karelz
karelz
commented
Jul 3, 2023

Copy link


Copy Markdown

Member
@antonfirsov ping, can you please take a look?
antonfirsov
antonfirsov
reviewed
Jul 4, 2023
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.

Sorry for the long delay!
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+2341
 to 
+2344

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 3, 2023

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.

Given that BeginSendFile calls into SendFileAsync, this check is redundant.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
 if (usePreAndPostbufferOverload)
 {
ex = await Assert.ThrowsAsync<SocketException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
 await Assert.ThrowsAsync<NotSupportedException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
@antonfirsovantonfirsovJul 3, 2023

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.

This test case should not be [PlatformSpecific(TestPlatforms.Windows)] anymore.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated


Comment on lines

+673
 to 
+676

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023•

edited

Loading

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.

Doesn't the check if (!IsConnectionOriented) do the job already? In #47472 I only tested the sync overload.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+1257
 to 
+1260

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023

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.

The async variant checks for if (!IsConnectionOriented) while the sync version checks (!Connected). I think that - instead of limiting the error case explicitly to UDP - we should consolidate these checks, throwing in case of (!IsConnectionOriented || !Connected).
@karelz
karelz
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@rhirano0715 will you have time to look at the CR feedback from @antonfirsov please?
rhirano0715and others
added 3 commits
July 19, 2023 18:06
@rhirano0715
Merge branch 'dotnet:main' into issue47472
aaf4917
@rhirano0715
Change to throws NotSupportedException if !IsConnectionOriented o…
a171986
…r `!Connected`.
Before:.
Throws `NotSupportedException` on UDP.
After:
Throws `NotSupportedException` if `!IsConnectionOriented` or `!Connected`.
@rhirano0715
Changed test case UdpConnection_ThrowsException to run regardless o…
7bd5d7c
…f platform.

This was referenced Jul 19, 2023

Closed


Closed

@rhirano0715
rhirano0715
commented
Jul 19, 2023

Copy link


Copy Markdown

ContributorAuthor
@karelz@antonfirsov

I'm sorry for not being able to respond.

Thank you for your review.

I have committed the correction.
karelz
karelz
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated

@rhirano0715@karelz
Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket…
3954c40
….Tasks.cs
Co-authored-by: Karel Zikmund <karelz@microsoft.com>
@antonfirsovantonfirsov
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Aug 4, 2023
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Aug 4, 2023
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, thanks, and sorry for the slow response. Started some extra CI runs just in case.

This was referenced Aug 4, 2023

Closed


Closed


Closed


Closed


Closed


This was referenced Aug 4, 2023

Closed


Closed

@MihaZupanMihaZupan
reopened this
Aug 8, 2023
@MihaZupan
MihaZupan
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 8, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 8, 2023

Closed

@MihaZupan
MihaZupan
merged commit 24f1513
into
dotnet:mainAug 9, 2023
@karelzkarelz
added this to the 8.0.0 milestone
Aug 10, 2023
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 9, 2023
@antonfirsovantonfirsov
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 17, 2023
@rhirano0715
rhirano0715
deleted the

issue47472

branch
December 6, 2023 08:21
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments

+1 more reviewer

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

Assignees

@antonfirsovantonfirsov

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Socket.SendFile() should fail with a consistent exception for connectionless sockets


4 participants

@rhirano0715@karelz@antonfirsov@MihaZupan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Unified to throw NotSupportedException when SendFile() for connectionless sockets by rhirano0715 · Pull Request #87108 · dotnet/runtime · GitHub
Skip to content
Unified to throw NotSupportedException when SendFile() for connectionless sockets - #87108
Merged
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Aug 9, 2023
Merged
Unified to throw NotSupportedException when SendFile() for connectionless sockets#87108
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Conversation
@rhirano0715
@rhirano0715rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

Contributor
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@rhirano0715
Unified to throw NotSupportedException when SendFile() for connection…
cb8c786
…less sockets
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP.
The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@ghostghost
added

area-System.Net.Sockets

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 4, 2023
@ghost
ghost
commented
Jun 4, 2023

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
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix #47472
Author:rhirano0715
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@rhirano0715
rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet-policy-service agree
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 4, 2023

Closed

@karelz
karelz
requested a review
from antonfirsovJune 27, 2023 13:18
@karelz
karelz
commented
Jul 3, 2023

Copy link


Copy Markdown

Member
@antonfirsov ping, can you please take a look?
antonfirsov
antonfirsov
reviewed
Jul 4, 2023
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.

Sorry for the long delay!
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+2341
 to 
+2344

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 3, 2023

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.

Given that BeginSendFile calls into SendFileAsync, this check is redundant.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
 if (usePreAndPostbufferOverload)
 {
ex = await Assert.ThrowsAsync<SocketException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
 await Assert.ThrowsAsync<NotSupportedException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
@antonfirsovantonfirsovJul 3, 2023

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.

This test case should not be [PlatformSpecific(TestPlatforms.Windows)] anymore.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated


Comment on lines

+673
 to 
+676

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023•

edited

Loading

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.

Doesn't the check if (!IsConnectionOriented) do the job already? In #47472 I only tested the sync overload.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+1257
 to 
+1260

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023

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.

The async variant checks for if (!IsConnectionOriented) while the sync version checks (!Connected). I think that - instead of limiting the error case explicitly to UDP - we should consolidate these checks, throwing in case of (!IsConnectionOriented || !Connected).
@karelz
karelz
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@rhirano0715 will you have time to look at the CR feedback from @antonfirsov please?
rhirano0715and others
added 3 commits
July 19, 2023 18:06
@rhirano0715
Merge branch 'dotnet:main' into issue47472
aaf4917
@rhirano0715
Change to throws NotSupportedException if !IsConnectionOriented o…
a171986
…r `!Connected`.
Before:.
Throws `NotSupportedException` on UDP.
After:
Throws `NotSupportedException` if `!IsConnectionOriented` or `!Connected`.
@rhirano0715
Changed test case UdpConnection_ThrowsException to run regardless o…
7bd5d7c
…f platform.

This was referenced Jul 19, 2023

Closed


Closed

@rhirano0715
rhirano0715
commented
Jul 19, 2023

Copy link


Copy Markdown

ContributorAuthor
@karelz@antonfirsov

I'm sorry for not being able to respond.

Thank you for your review.

I have committed the correction.
karelz
karelz
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated

@rhirano0715@karelz
Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket…
3954c40
….Tasks.cs
Co-authored-by: Karel Zikmund <karelz@microsoft.com>
@antonfirsovantonfirsov
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Aug 4, 2023
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Aug 4, 2023
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, thanks, and sorry for the slow response. Started some extra CI runs just in case.

This was referenced Aug 4, 2023

Closed


Closed


Closed


Closed


Closed


This was referenced Aug 4, 2023

Closed


Closed

@MihaZupanMihaZupan
reopened this
Aug 8, 2023
@MihaZupan
MihaZupan
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 8, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 8, 2023

Closed

@MihaZupan
MihaZupan
merged commit 24f1513
into
dotnet:mainAug 9, 2023
@karelzkarelz
added this to the 8.0.0 milestone
Aug 10, 2023
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 9, 2023
@antonfirsovantonfirsov
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 17, 2023
@rhirano0715
rhirano0715
deleted the

issue47472

branch
December 6, 2023 08:21
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments

+1 more reviewer

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

Assignees

@antonfirsovantonfirsov

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Socket.SendFile() should fail with a consistent exception for connectionless sockets


4 participants

@rhirano0715@karelz@antonfirsov@MihaZupan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Unified to throw NotSupportedException when SendFile() for connectionless sockets by rhirano0715 · Pull Request #87108 · dotnet/runtime · GitHub
Skip to content
Unified to throw NotSupportedException when SendFile() for connectionless sockets - #87108
Merged
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Aug 9, 2023
Merged
Unified to throw NotSupportedException when SendFile() for connectionless sockets#87108
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Conversation
@rhirano0715
@rhirano0715rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

Contributor
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@rhirano0715
Unified to throw NotSupportedException when SendFile() for connection…
cb8c786
…less sockets
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP.
The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@ghostghost
added

area-System.Net.Sockets

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 4, 2023
@ghost
ghost
commented
Jun 4, 2023

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
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix #47472
Author:rhirano0715
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@rhirano0715
rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet-policy-service agree
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 4, 2023

Closed

@karelz
karelz
requested a review
from antonfirsovJune 27, 2023 13:18
@karelz
karelz
commented
Jul 3, 2023

Copy link


Copy Markdown

Member
@antonfirsov ping, can you please take a look?
antonfirsov
antonfirsov
reviewed
Jul 4, 2023
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.

Sorry for the long delay!
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+2341
 to 
+2344

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 3, 2023

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.

Given that BeginSendFile calls into SendFileAsync, this check is redundant.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
 if (usePreAndPostbufferOverload)
 {
ex = await Assert.ThrowsAsync<SocketException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
 await Assert.ThrowsAsync<NotSupportedException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
@antonfirsovantonfirsovJul 3, 2023

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.

This test case should not be [PlatformSpecific(TestPlatforms.Windows)] anymore.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated


Comment on lines

+673
 to 
+676

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023•

edited

Loading

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.

Doesn't the check if (!IsConnectionOriented) do the job already? In #47472 I only tested the sync overload.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+1257
 to 
+1260

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023

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.

The async variant checks for if (!IsConnectionOriented) while the sync version checks (!Connected). I think that - instead of limiting the error case explicitly to UDP - we should consolidate these checks, throwing in case of (!IsConnectionOriented || !Connected).
@karelz
karelz
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@rhirano0715 will you have time to look at the CR feedback from @antonfirsov please?
rhirano0715and others
added 3 commits
July 19, 2023 18:06
@rhirano0715
Merge branch 'dotnet:main' into issue47472
aaf4917
@rhirano0715
Change to throws NotSupportedException if !IsConnectionOriented o…
a171986
…r `!Connected`.
Before:.
Throws `NotSupportedException` on UDP.
After:
Throws `NotSupportedException` if `!IsConnectionOriented` or `!Connected`.
@rhirano0715
Changed test case UdpConnection_ThrowsException to run regardless o…
7bd5d7c
…f platform.

This was referenced Jul 19, 2023

Closed


Closed

@rhirano0715
rhirano0715
commented
Jul 19, 2023

Copy link


Copy Markdown

ContributorAuthor
@karelz@antonfirsov

I'm sorry for not being able to respond.

Thank you for your review.

I have committed the correction.
karelz
karelz
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated

@rhirano0715@karelz
Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket…
3954c40
….Tasks.cs
Co-authored-by: Karel Zikmund <karelz@microsoft.com>
@antonfirsovantonfirsov
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Aug 4, 2023
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Aug 4, 2023
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, thanks, and sorry for the slow response. Started some extra CI runs just in case.

This was referenced Aug 4, 2023

Closed


Closed


Closed


Closed


Closed


This was referenced Aug 4, 2023

Closed


Closed

@MihaZupanMihaZupan
reopened this
Aug 8, 2023
@MihaZupan
MihaZupan
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 8, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 8, 2023

Closed

@MihaZupan
MihaZupan
merged commit 24f1513
into
dotnet:mainAug 9, 2023
@karelzkarelz
added this to the 8.0.0 milestone
Aug 10, 2023
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 9, 2023
@antonfirsovantonfirsov
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 17, 2023
@rhirano0715
rhirano0715
deleted the

issue47472

branch
December 6, 2023 08:21
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments

+1 more reviewer

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

Assignees

@antonfirsovantonfirsov

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Socket.SendFile() should fail with a consistent exception for connectionless sockets


4 participants

@rhirano0715@karelz@antonfirsov@MihaZupan

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Unified to throw NotSupportedException when SendFile() for connectionless sockets by rhirano0715 · Pull Request #87108 · dotnet/runtime · GitHub
Skip to content
Unified to throw NotSupportedException when SendFile() for connectionless sockets - #87108
Merged
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Aug 9, 2023
Merged
Unified to throw NotSupportedException when SendFile() for connectionless sockets#87108
MihaZupan merged 5 commits into
dotnet:mainfrom 
rhirano0715:issue47472
Conversation
@rhirano0715
@rhirano0715rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

Contributor
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@rhirano0715
Unified to throw NotSupportedException when SendFile() for connection…
cb8c786
…less sockets
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP.
The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@ghostghost
added

area-System.Net.Sockets

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 4, 2023
@ghost
ghost
commented
Jun 4, 2023

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
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix #47472
Author:rhirano0715
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@rhirano0715
rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet-policy-service agree
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 4, 2023

Closed

@karelz
karelz
requested a review
from antonfirsovJune 27, 2023 13:18
@karelz
karelz
commented
Jul 3, 2023

Copy link


Copy Markdown

Member
@antonfirsov ping, can you please take a look?
antonfirsov
antonfirsov
reviewed
Jul 4, 2023
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.

Sorry for the long delay!
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+2341
 to 
+2344

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 3, 2023

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.

Given that BeginSendFile calls into SendFileAsync, this check is redundant.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
 if (usePreAndPostbufferOverload)
 {
ex = await Assert.ThrowsAsync<SocketException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
 await Assert.ThrowsAsync<NotSupportedException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
@antonfirsovantonfirsovJul 3, 2023

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.

This test case should not be [PlatformSpecific(TestPlatforms.Windows)] anymore.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated


Comment on lines

+673
 to 
+676

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023•

edited

Loading

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.

Doesn't the check if (!IsConnectionOriented) do the job already? In #47472 I only tested the sync overload.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+1257
 to 
+1260

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023

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.

The async variant checks for if (!IsConnectionOriented) while the sync version checks (!Connected). I think that - instead of limiting the error case explicitly to UDP - we should consolidate these checks, throwing in case of (!IsConnectionOriented || !Connected).
@karelz
karelz
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@rhirano0715 will you have time to look at the CR feedback from @antonfirsov please?
rhirano0715and others
added 3 commits
July 19, 2023 18:06
@rhirano0715
Merge branch 'dotnet:main' into issue47472
aaf4917
@rhirano0715
Change to throws NotSupportedException if !IsConnectionOriented o…
a171986
…r `!Connected`.
Before:.
Throws `NotSupportedException` on UDP.
After:
Throws `NotSupportedException` if `!IsConnectionOriented` or `!Connected`.
@rhirano0715
Changed test case UdpConnection_ThrowsException to run regardless o…
7bd5d7c
…f platform.

This was referenced Jul 19, 2023

Closed


Closed

@rhirano0715
rhirano0715
commented
Jul 19, 2023

Copy link


Copy Markdown

ContributorAuthor
@karelz@antonfirsov

I'm sorry for not being able to respond.

Thank you for your review.

I have committed the correction.
karelz
karelz
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated

@rhirano0715@karelz
Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket…
3954c40
….Tasks.cs
Co-authored-by: Karel Zikmund <karelz@microsoft.com>
@antonfirsovantonfirsov
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Aug 4, 2023
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Aug 4, 2023
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, thanks, and sorry for the slow response. Started some extra CI runs just in case.

This was referenced Aug 4, 2023

Closed


Closed


Closed


Closed


Closed


This was referenced Aug 4, 2023

Closed


Closed

@MihaZupanMihaZupan
reopened this
Aug 8, 2023
@MihaZupan
MihaZupan
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 8, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 8, 2023

Closed

@MihaZupan
MihaZupan
merged commit 24f1513
into
dotnet:mainAug 9, 2023
@karelzkarelz
added this to the 8.0.0 milestone
Aug 10, 2023
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 9, 2023
@antonfirsovantonfirsov
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 17, 2023
@rhirano0715
rhirano0715
deleted the

issue47472

branch
December 6, 2023 08:21
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments

+1 more reviewer

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

Assignees

@antonfirsovantonfirsov

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Socket.SendFile() should fail with a consistent exception for connectionless sockets


4 participants

@rhirano0715@karelz@antonfirsov@MihaZupan

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

Copy link


Copy Markdown

Contributor
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@rhirano0715
Unified to throw NotSupportedException when SendFile() for connection…
cb8c786
…less sockets
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP.
The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix#47472
@ghostghost
added

area-System.Net.Sockets

community-contribution
Indicates that the PR has been added by a community member
labels
Jun 4, 2023
@ghost
ghost
commented
Jun 4, 2023

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
The issue was that the Socket.SendFile() threw inconsistent exceptions when the platform was Windows and the protocol was UDP. The first call would throw a SocketException, while the second call would throw a NotSupportedException.
With this commit, SendFile() will consistently throw NotSupportException on all platforms when the protocol is UDP.
Fix #47472
Author:rhirano0715
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@rhirano0715
rhirano0715
commented
Jun 4, 2023

Copy link


Copy Markdown

ContributorAuthor
@dotnet-policy-service agree
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 4, 2023

Closed

@karelz
karelz
requested a review
from antonfirsovJune 27, 2023 13:18
@karelz
karelz
commented
Jul 3, 2023

Copy link


Copy Markdown

Member
@antonfirsov ping, can you please take a look?
antonfirsov
antonfirsov
reviewed
Jul 4, 2023
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.

Sorry for the long delay!
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+2341
 to 
+2344

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 3, 2023

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.

Given that BeginSendFile calls into SendFileAsync, this check is redundant.
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/SendFile.cs
 if (usePreAndPostbufferOverload)
 {
ex = await Assert.ThrowsAsync<SocketException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
 await Assert.ThrowsAsync<NotSupportedException>(() => SendFileAsync(client, tempFile.Path, Array.Empty<byte>(), Array.Empty<byte>(), TransmitFileOptions.UseDefaultWorkerThread));
@antonfirsovantonfirsovJul 3, 2023

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.

This test case should not be [PlatformSpecific(TestPlatforms.Windows)] anymore.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated


Comment on lines

+673
 to 
+676

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023•

edited

Loading

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.

Doesn't the check if (!IsConnectionOriented) do the job already? In #47472 I only tested the sync overload.
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated


Comment on lines

+1257
 to 
+1260

 if (_protocolType == ProtocolType.Udp)
 {
 throw new NotSupportedException(SR.net_notconnected);
 }
@antonfirsovantonfirsovJul 4, 2023

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.

The async variant checks for if (!IsConnectionOriented) while the sync version checks (!Connected). I think that - instead of limiting the error case explicitly to UDP - we should consolidate these checks, throwing in case of (!IsConnectionOriented || !Connected).
@karelz
karelz
commented
Jul 18, 2023

Copy link


Copy Markdown

Member
@rhirano0715 will you have time to look at the CR feedback from @antonfirsov please?
rhirano0715and others
added 3 commits
July 19, 2023 18:06
@rhirano0715
Merge branch 'dotnet:main' into issue47472
aaf4917
@rhirano0715
Change to throws NotSupportedException if !IsConnectionOriented o…
a171986
…r `!Connected`.
Before:.
Throws `NotSupportedException` on UDP.
After:
Throws `NotSupportedException` if `!IsConnectionOriented` or `!Connected`.
@rhirano0715
Changed test case UdpConnection_ThrowsException to run regardless o…
7bd5d7c
…f platform.

This was referenced Jul 19, 2023

Closed


Closed

@rhirano0715
rhirano0715
commented
Jul 19, 2023

Copy link


Copy Markdown

ContributorAuthor
@karelz@antonfirsov

I'm sorry for not being able to respond.

Thank you for your review.

I have committed the correction.
karelz
karelz
reviewed
Aug 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs
Outdated

@rhirano0715@karelz
Update src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket…
3954c40
….Tasks.cs
Co-authored-by: Karel Zikmund <karelz@microsoft.com>
@antonfirsovantonfirsov
added
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Aug 4, 2023
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime
@antonfirsov
antonfirsov
commented
Aug 4, 2023

Copy link


Copy Markdown

Contributor
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment

@azure-pipelines
azure-pipelinesBot
commented
Aug 4, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
Aug 4, 2023
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, thanks, and sorry for the slow response. Started some extra CI runs just in case.

This was referenced Aug 4, 2023

Closed


Closed


Closed


Closed


Closed


This was referenced Aug 4, 2023

Closed


Closed

@MihaZupanMihaZupan
reopened this
Aug 8, 2023
@MihaZupan
MihaZupan
commented
Aug 8, 2023

Copy link


Copy Markdown

Member
/azp run runtime-extra-platforms
@azure-pipelines
azure-pipelinesBot
commented
Aug 8, 2023

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@build-analysisbuild-analysisBot
mentioned this pull request
Aug 8, 2023

Closed

@MihaZupan
MihaZupan
merged commit 24f1513
into
dotnet:mainAug 9, 2023
@karelzkarelz
added this to the 8.0.0 milestone
Aug 10, 2023
@ghostghost
locked as resolved and limited conversation to collaborators
Sep 9, 2023
@antonfirsovantonfirsov
removed
the 
needs-breaking-change-doc-created
Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
label
Oct 17, 2023
@rhirano0715
rhirano0715
deleted the

issue47472

branch
December 6, 2023 08:21
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@karelzkarelzkarelz left review comments

+1 more reviewer

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

Assignees

@antonfirsovantonfirsov

Labels

area-System.Net.Socketscommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

Socket.SendFile() should fail with a consistent exception for connectionless sockets


4 participants

@rhirano0715@karelz@antonfirsov@MihaZupan