Socket: delete unix local endpoint filename on Close - #52103

Merged
antonfirsov merged 13 commits into
dotnet:mainfrom
tmds:unix_socket_delete_file
May 31, 2021
Merged

Socket: delete unix local endpoint filename on Close#52103
antonfirsov merged 13 commits into
dotnet:mainfrom
tmds:unix_socket_delete_file

Conversation

@tmds

@tmdstmds commented Apr 30, 2021

Copy link
Copy Markdown
Member

@ghost

ghost commented Apr 30, 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

Fixes #45537

@antonfirsov@geoffkizer ptal

Author:tmds
Assignees:-
Labels:

area-System.Net.Sockets

Milestone:-

@tmds

tmds commented Apr 30, 2021

Copy link
Copy Markdown
MemberAuthor

I just realized relative paths need specific handling. The working directory may change between the time of Bind and Dispose. I'll update the PR for this, no need to review it yet.

@wfurt

wfurt commented Apr 30, 2021

Copy link
Copy Markdown
Member

should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?

@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@tmds

tmds commented May 3, 2021

Copy link
Copy Markdown
MemberAuthor

should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?

The file is created when the socket is bound. If the file is already there, the socket will fail to bind.

tmds
tmds commented May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
// correct type (IPEndPoint, etc). The Bind operation sets _rightEndPoint. Other operations must only set
// it when the value is still null.
// This enables tracking the file created by UnixDomainSocketEndPoint when the Socket is bound,
// and to delete that file when the Socket gets disposed.

@tmdstmdsMay 3, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I avoided adding a field to Socket for the stake of tracking the file to be deleted. Instead this is tracked through _rightEndPointUnixDomainSocketEndPoint.BoundFileName.
On Bind an instance is created that has this property set. And on dispose, the file then gets deleted.

@stephentoubstephentoubMay 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've wondered this for a while... what does the "right" part of the name mean?

@tmdstmdsMay 25, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Just guessing: right type. The name is not very clear. Maybe we should change it to _typedEndpoint or _endpointType?

@antonfirsovantonfirsovMay 25, 2021

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.

I keep asking this question every time I see that variable.

@geoffkizergeoffkizerMay 25, 2021

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.

I don't really know either. Would love to understand.

It might be "right" in the sense that it's the result after resolving wildcards like port=0. Does that seem right?

tmds
tmds commented May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs Outdated
tmds
tmds commented May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
@tmds

tmds commented May 11, 2021

Copy link
Copy Markdown
MemberAuthor

@antonfirsov@geoffkizer can you continue reviewing this?

@antonfirsov

antonfirsov commented May 11, 2021

Copy link
Copy Markdown
Contributor

@tmds the UnixDomainSocketEndPoint changes look good to me. I'm only concerned about caching optimizations (discussion above).

@karelz

karelz commented May 18, 2021

Copy link
Copy Markdown
Member

Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.

@tmds

tmds commented May 18, 2021

Copy link
Copy Markdown
MemberAuthor

I need to look at the CI failures, they seem related to the change.

Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.

The discussion is more about #39313 than it is about this PR. Can we remove the optimization if it is decided to revert that PR? It will then happen as part of removing the _localEndPoint field that was introduced in #39313. @antonfirsov wdyt?

@antonfirsov

antonfirsov commented May 19, 2021

Copy link
Copy Markdown
Contributor

@tmds we are undecided yet re reverting _localEndpoint I plan to open a separate issue to continue discussions on that topic. The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.

I need to look at the CI failures, they seem related to the change.

I can't see the failures, I assume it was outerloop which disappeared from the log, will rerun those tests.

@antonfirsov

antonfirsov commented May 19, 2021

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented May 19, 2021

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

@tmds

tmds commented May 19, 2021

Copy link
Copy Markdown
MemberAuthor

The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.

The concern isn't about how this PR uses _localEndpoint but whether there should be a _localEndpoint. I don't see why I shouldn't use it while it is here, but I'll remove it based on your feedback.

tmds
tmds commented May 20, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs Outdated
@runfoapprunfoappBot mentioned this pull request May 20, 2021
Closed
stephentoub
stephentoub reviewed May 25, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
// Update the state if we've become connected after a non-blocking connect.
_isConnected = true;
_rightEndPoint = _nonBlockingConnectRightEndPoint;
_rightEndPoint ??= _nonBlockingConnectRightEndPoint;

@stephentoubstephentoubMay 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In what situation will it be non-null? I'm wondering in what situation we previously would have overwritten it and now we won't...

@tmdstmdsMay 25, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I didn't think about this much. I replaced everything with a conditional assignment except for the one in Bind.

It can be non-null if for example you Bind first, and then do a Connect and then call LocalEndPoint and end up in this branch.
We don't want to overwrite it because that will lose the tracking (via _rightEndPoint) of the unix socket file we need to delete.

@antonfirsovantonfirsovMay 25, 2021•
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.

Maybe it would make sense to add a test for this case, so we don't break it in the future. Scratch it would be too difficult because of timing.

antonfirsov
antonfirsov reviewed May 25, 2021
View reviewed changes

@antonfirsovantonfirsov left a comment •
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.

Looks good after fixing the test (https://github.com/dotnet/runtime/pull/52103/files#r638644278) and answering https://github.com/dotnet/runtime/pull/52103/files#r638385763 (missed the answer).

@antonfirsov

antonfirsov commented May 25, 2021

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented May 25, 2021

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

antonfirsov
antonfirsov approved these changes May 26, 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. CI failures are unrelated build failures.

@antonfirsov
antonfirsov merged commit d3ed5a9 into dotnet:mainMay 31, 2021
@tmdstmds mentioned this pull request May 31, 2021
Closed
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@ghostghost locked as resolved and limited conversation to collaborators Jun 30, 2021
@karelzkarelz added this to the 6.0.0 milestone Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub left review comments
+2 more reviewers
@geoffkizergeoffkizergeoffkizer left review comments
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Sockets

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

proposal: delete unix socket path on Dispose of bound Socket

6 participants

@tmds@wfurt@antonfirsov@karelz@stephentoub@geoffkizer
, '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
Socket: delete unix local endpoint filename on Close - #52103
Merged
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
May 31, 2021
Merged
Socket: delete unix local endpoint filename on Close#52103
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
Conversation
@tmds
@tmdstmds
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
@ghost
ghost
commented
Apr 30, 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
Fixes #45537
@antonfirsov@geoffkizer ptal
Author:tmds
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@tmds
tmds
commented
Apr 30, 2021

Copy link


Copy Markdown

MemberAuthor
I just realized relative paths need specific handling. The working directory may change between the time of Bind and Dispose. I'll update the PR for this, no need to review it yet.
@wfurt
wfurt
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@tmds
tmds
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
The file is created when the socket is bound. If the file is already there, the socket will fail to bind.
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // correct type (IPEndPoint, etc). The Bind operation sets _rightEndPoint. Other operations must only set
 // it when the value is still null.
 // This enables tracking the file created by UnixDomainSocketEndPoint when the Socket is bound,
 // and to delete that file when the Socket gets disposed.
@tmdstmdsMay 3, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I avoided adding a field to Socket for the stake of tracking the file to be deleted. Instead this is tracked through _rightEndPointUnixDomainSocketEndPoint.BoundFileName.

On Bind an instance is created that has this property set. And on dispose, the file then gets deleted.
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've wondered this for a while... what does the "right" part of the name mean?
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Just guessing: right type. The name is not very clear. Maybe we should change it to _typedEndpoint or _endpointType?
@antonfirsovantonfirsovMay 25, 2021

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.

I keep asking this question every time I see that variable.
@geoffkizergeoffkizerMay 25, 2021

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.

I don't really know either. Would love to understand.
It might be "right" in the sense that it's the result after resolving wildcards like port=0. Does that seem right?
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated

tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
@tmds
tmds
commented
May 11, 2021

Copy link


Copy Markdown

MemberAuthor
@antonfirsov@geoffkizer can you continue reviewing this?
@antonfirsov
antonfirsov
commented
May 11, 2021

Copy link


Copy Markdown

Contributor
@tmds the UnixDomainSocketEndPoint changes look good to me. I'm only concerned about caching optimizations (discussion above).
@karelz
karelz
commented
May 18, 2021

Copy link


Copy Markdown

Member
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
@tmds
tmds
commented
May 18, 2021

Copy link


Copy Markdown

MemberAuthor
I need to look at the CI failures, they seem related to the change.
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
The discussion is more about #39313 than it is about this PR. Can we remove the optimization if it is decided to revert that PR? It will then happen as part of removing the _localEndPoint field that was introduced in #39313. @antonfirsov wdyt?
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

Contributor
@tmds we are undecided yet re reverting _localEndpoint I plan to open a separate issue to continue discussions on that topic. The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
I need to look at the CI failures, they seem related to the change.
I can't see the failures, I assume it was outerloop which disappeared from the log, will rerun those tests.
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@tmds
tmds
commented
May 19, 2021

Copy link


Copy Markdown

MemberAuthor
The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
The concern isn't about how this PR uses _localEndpoint but whether there should be a _localEndpoint. I don't see why I shouldn't use it while it is here, but I'll remove it based on your feedback.
tmds
tmds
commented
May 20, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs
Outdated

@runfoapprunfoappBot
mentioned this pull request
May 20, 2021

Closed

stephentoub
stephentoub
reviewed
May 25, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // Update the state if we've become connected after a non-blocking connect.
 _isConnected = true;
 _rightEndPoint = _nonBlockingConnectRightEndPoint;
 _rightEndPoint ??= _nonBlockingConnectRightEndPoint;
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In what situation will it be non-null? I'm wondering in what situation we previously would have overwritten it and now we won't...
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I didn't think about this much. I replaced everything with a conditional assignment except for the one in Bind.
It can be non-null if for example you Bind first, and then do a Connect and then call LocalEndPoint and end up in this branch.

We don't want to overwrite it because that will lose the tracking (via _rightEndPoint) of the unix socket file we need to delete.
@antonfirsovantonfirsovMay 25, 2021•

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.

Maybe it would make sense to add a test for this case, so we don't break it in the future. Scratch it would be too difficult because of timing.
antonfirsov
antonfirsov
reviewed
May 25, 2021
View reviewed changes
@antonfirsovantonfirsov
left a comment
•

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.

Looks good after fixing the test (https://github.com/dotnet/runtime/pull/52103/files#r638644278) and answering https://github.com/dotnet/runtime/pull/52103/files#r638385763 (missed the answer).
@antonfirsov
antonfirsov
commented
May 25, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
May 26, 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. CI failures are unrelated build failures.
@antonfirsov
antonfirsov
merged commit d3ed5a9
into
dotnet:mainMay 31, 2021
@tmdstmds
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Sockets

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

proposal: delete unix socket path on Dispose of bound Socket


6 participants

@tmds@wfurt@antonfirsov@karelz@stephentoub@geoffkizer

, '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
Socket: delete unix local endpoint filename on Close - #52103
Merged
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
May 31, 2021
Merged
Socket: delete unix local endpoint filename on Close#52103
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
Conversation
@tmds
@tmdstmds
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
@ghost
ghost
commented
Apr 30, 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
Fixes #45537
@antonfirsov@geoffkizer ptal
Author:tmds
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@tmds
tmds
commented
Apr 30, 2021

Copy link


Copy Markdown

MemberAuthor
I just realized relative paths need specific handling. The working directory may change between the time of Bind and Dispose. I'll update the PR for this, no need to review it yet.
@wfurt
wfurt
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@tmds
tmds
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
The file is created when the socket is bound. If the file is already there, the socket will fail to bind.
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // correct type (IPEndPoint, etc). The Bind operation sets _rightEndPoint. Other operations must only set
 // it when the value is still null.
 // This enables tracking the file created by UnixDomainSocketEndPoint when the Socket is bound,
 // and to delete that file when the Socket gets disposed.
@tmdstmdsMay 3, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I avoided adding a field to Socket for the stake of tracking the file to be deleted. Instead this is tracked through _rightEndPointUnixDomainSocketEndPoint.BoundFileName.

On Bind an instance is created that has this property set. And on dispose, the file then gets deleted.
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've wondered this for a while... what does the "right" part of the name mean?
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Just guessing: right type. The name is not very clear. Maybe we should change it to _typedEndpoint or _endpointType?
@antonfirsovantonfirsovMay 25, 2021

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.

I keep asking this question every time I see that variable.
@geoffkizergeoffkizerMay 25, 2021

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.

I don't really know either. Would love to understand.
It might be "right" in the sense that it's the result after resolving wildcards like port=0. Does that seem right?
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated

tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
@tmds
tmds
commented
May 11, 2021

Copy link


Copy Markdown

MemberAuthor
@antonfirsov@geoffkizer can you continue reviewing this?
@antonfirsov
antonfirsov
commented
May 11, 2021

Copy link


Copy Markdown

Contributor
@tmds the UnixDomainSocketEndPoint changes look good to me. I'm only concerned about caching optimizations (discussion above).
@karelz
karelz
commented
May 18, 2021

Copy link


Copy Markdown

Member
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
@tmds
tmds
commented
May 18, 2021

Copy link


Copy Markdown

MemberAuthor
I need to look at the CI failures, they seem related to the change.
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
The discussion is more about #39313 than it is about this PR. Can we remove the optimization if it is decided to revert that PR? It will then happen as part of removing the _localEndPoint field that was introduced in #39313. @antonfirsov wdyt?
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

Contributor
@tmds we are undecided yet re reverting _localEndpoint I plan to open a separate issue to continue discussions on that topic. The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
I need to look at the CI failures, they seem related to the change.
I can't see the failures, I assume it was outerloop which disappeared from the log, will rerun those tests.
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@tmds
tmds
commented
May 19, 2021

Copy link


Copy Markdown

MemberAuthor
The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
The concern isn't about how this PR uses _localEndpoint but whether there should be a _localEndpoint. I don't see why I shouldn't use it while it is here, but I'll remove it based on your feedback.
tmds
tmds
commented
May 20, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs
Outdated

@runfoapprunfoappBot
mentioned this pull request
May 20, 2021

Closed

stephentoub
stephentoub
reviewed
May 25, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // Update the state if we've become connected after a non-blocking connect.
 _isConnected = true;
 _rightEndPoint = _nonBlockingConnectRightEndPoint;
 _rightEndPoint ??= _nonBlockingConnectRightEndPoint;
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In what situation will it be non-null? I'm wondering in what situation we previously would have overwritten it and now we won't...
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I didn't think about this much. I replaced everything with a conditional assignment except for the one in Bind.
It can be non-null if for example you Bind first, and then do a Connect and then call LocalEndPoint and end up in this branch.

We don't want to overwrite it because that will lose the tracking (via _rightEndPoint) of the unix socket file we need to delete.
@antonfirsovantonfirsovMay 25, 2021•

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.

Maybe it would make sense to add a test for this case, so we don't break it in the future. Scratch it would be too difficult because of timing.
antonfirsov
antonfirsov
reviewed
May 25, 2021
View reviewed changes
@antonfirsovantonfirsov
left a comment
•

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.

Looks good after fixing the test (https://github.com/dotnet/runtime/pull/52103/files#r638644278) and answering https://github.com/dotnet/runtime/pull/52103/files#r638385763 (missed the answer).
@antonfirsov
antonfirsov
commented
May 25, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
May 26, 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. CI failures are unrelated build failures.
@antonfirsov
antonfirsov
merged commit d3ed5a9
into
dotnet:mainMay 31, 2021
@tmdstmds
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Sockets

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

proposal: delete unix socket path on Dispose of bound Socket


6 participants

@tmds@wfurt@antonfirsov@karelz@stephentoub@geoffkizer

, '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
Socket: delete unix local endpoint filename on Close - #52103
Merged
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
May 31, 2021
Merged
Socket: delete unix local endpoint filename on Close#52103
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
Conversation
@tmds
@tmdstmds
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
@ghost
ghost
commented
Apr 30, 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
Fixes #45537
@antonfirsov@geoffkizer ptal
Author:tmds
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@tmds
tmds
commented
Apr 30, 2021

Copy link


Copy Markdown

MemberAuthor
I just realized relative paths need specific handling. The working directory may change between the time of Bind and Dispose. I'll update the PR for this, no need to review it yet.
@wfurt
wfurt
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@tmds
tmds
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
The file is created when the socket is bound. If the file is already there, the socket will fail to bind.
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // correct type (IPEndPoint, etc). The Bind operation sets _rightEndPoint. Other operations must only set
 // it when the value is still null.
 // This enables tracking the file created by UnixDomainSocketEndPoint when the Socket is bound,
 // and to delete that file when the Socket gets disposed.
@tmdstmdsMay 3, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I avoided adding a field to Socket for the stake of tracking the file to be deleted. Instead this is tracked through _rightEndPointUnixDomainSocketEndPoint.BoundFileName.

On Bind an instance is created that has this property set. And on dispose, the file then gets deleted.
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've wondered this for a while... what does the "right" part of the name mean?
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Just guessing: right type. The name is not very clear. Maybe we should change it to _typedEndpoint or _endpointType?
@antonfirsovantonfirsovMay 25, 2021

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.

I keep asking this question every time I see that variable.
@geoffkizergeoffkizerMay 25, 2021

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.

I don't really know either. Would love to understand.
It might be "right" in the sense that it's the result after resolving wildcards like port=0. Does that seem right?
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated

tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
@tmds
tmds
commented
May 11, 2021

Copy link


Copy Markdown

MemberAuthor
@antonfirsov@geoffkizer can you continue reviewing this?
@antonfirsov
antonfirsov
commented
May 11, 2021

Copy link


Copy Markdown

Contributor
@tmds the UnixDomainSocketEndPoint changes look good to me. I'm only concerned about caching optimizations (discussion above).
@karelz
karelz
commented
May 18, 2021

Copy link


Copy Markdown

Member
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
@tmds
tmds
commented
May 18, 2021

Copy link


Copy Markdown

MemberAuthor
I need to look at the CI failures, they seem related to the change.
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
The discussion is more about #39313 than it is about this PR. Can we remove the optimization if it is decided to revert that PR? It will then happen as part of removing the _localEndPoint field that was introduced in #39313. @antonfirsov wdyt?
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

Contributor
@tmds we are undecided yet re reverting _localEndpoint I plan to open a separate issue to continue discussions on that topic. The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
I need to look at the CI failures, they seem related to the change.
I can't see the failures, I assume it was outerloop which disappeared from the log, will rerun those tests.
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@tmds
tmds
commented
May 19, 2021

Copy link


Copy Markdown

MemberAuthor
The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
The concern isn't about how this PR uses _localEndpoint but whether there should be a _localEndpoint. I don't see why I shouldn't use it while it is here, but I'll remove it based on your feedback.
tmds
tmds
commented
May 20, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs
Outdated

@runfoapprunfoappBot
mentioned this pull request
May 20, 2021

Closed

stephentoub
stephentoub
reviewed
May 25, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // Update the state if we've become connected after a non-blocking connect.
 _isConnected = true;
 _rightEndPoint = _nonBlockingConnectRightEndPoint;
 _rightEndPoint ??= _nonBlockingConnectRightEndPoint;
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In what situation will it be non-null? I'm wondering in what situation we previously would have overwritten it and now we won't...
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I didn't think about this much. I replaced everything with a conditional assignment except for the one in Bind.
It can be non-null if for example you Bind first, and then do a Connect and then call LocalEndPoint and end up in this branch.

We don't want to overwrite it because that will lose the tracking (via _rightEndPoint) of the unix socket file we need to delete.
@antonfirsovantonfirsovMay 25, 2021•

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.

Maybe it would make sense to add a test for this case, so we don't break it in the future. Scratch it would be too difficult because of timing.
antonfirsov
antonfirsov
reviewed
May 25, 2021
View reviewed changes
@antonfirsovantonfirsov
left a comment
•

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.

Looks good after fixing the test (https://github.com/dotnet/runtime/pull/52103/files#r638644278) and answering https://github.com/dotnet/runtime/pull/52103/files#r638385763 (missed the answer).
@antonfirsov
antonfirsov
commented
May 25, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
May 26, 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. CI failures are unrelated build failures.
@antonfirsov
antonfirsov
merged commit d3ed5a9
into
dotnet:mainMay 31, 2021
@tmdstmds
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Sockets

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

proposal: delete unix socket path on Dispose of bound Socket


6 participants

@tmds@wfurt@antonfirsov@karelz@stephentoub@geoffkizer

, '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
Socket: delete unix local endpoint filename on Close - #52103
Merged
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
May 31, 2021
Merged
Socket: delete unix local endpoint filename on Close#52103
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
Conversation
@tmds
@tmdstmds
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
@ghost
ghost
commented
Apr 30, 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
Fixes #45537
@antonfirsov@geoffkizer ptal
Author:tmds
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@tmds
tmds
commented
Apr 30, 2021

Copy link


Copy Markdown

MemberAuthor
I just realized relative paths need specific handling. The working directory may change between the time of Bind and Dispose. I'll update the PR for this, no need to review it yet.
@wfurt
wfurt
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@tmds
tmds
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
The file is created when the socket is bound. If the file is already there, the socket will fail to bind.
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // correct type (IPEndPoint, etc). The Bind operation sets _rightEndPoint. Other operations must only set
 // it when the value is still null.
 // This enables tracking the file created by UnixDomainSocketEndPoint when the Socket is bound,
 // and to delete that file when the Socket gets disposed.
@tmdstmdsMay 3, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I avoided adding a field to Socket for the stake of tracking the file to be deleted. Instead this is tracked through _rightEndPointUnixDomainSocketEndPoint.BoundFileName.

On Bind an instance is created that has this property set. And on dispose, the file then gets deleted.
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've wondered this for a while... what does the "right" part of the name mean?
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Just guessing: right type. The name is not very clear. Maybe we should change it to _typedEndpoint or _endpointType?
@antonfirsovantonfirsovMay 25, 2021

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.

I keep asking this question every time I see that variable.
@geoffkizergeoffkizerMay 25, 2021

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.

I don't really know either. Would love to understand.
It might be "right" in the sense that it's the result after resolving wildcards like port=0. Does that seem right?
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated

tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
@tmds
tmds
commented
May 11, 2021

Copy link


Copy Markdown

MemberAuthor
@antonfirsov@geoffkizer can you continue reviewing this?
@antonfirsov
antonfirsov
commented
May 11, 2021

Copy link


Copy Markdown

Contributor
@tmds the UnixDomainSocketEndPoint changes look good to me. I'm only concerned about caching optimizations (discussion above).
@karelz
karelz
commented
May 18, 2021

Copy link


Copy Markdown

Member
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
@tmds
tmds
commented
May 18, 2021

Copy link


Copy Markdown

MemberAuthor
I need to look at the CI failures, they seem related to the change.
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
The discussion is more about #39313 than it is about this PR. Can we remove the optimization if it is decided to revert that PR? It will then happen as part of removing the _localEndPoint field that was introduced in #39313. @antonfirsov wdyt?
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

Contributor
@tmds we are undecided yet re reverting _localEndpoint I plan to open a separate issue to continue discussions on that topic. The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
I need to look at the CI failures, they seem related to the change.
I can't see the failures, I assume it was outerloop which disappeared from the log, will rerun those tests.
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@tmds
tmds
commented
May 19, 2021

Copy link


Copy Markdown

MemberAuthor
The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
The concern isn't about how this PR uses _localEndpoint but whether there should be a _localEndpoint. I don't see why I shouldn't use it while it is here, but I'll remove it based on your feedback.
tmds
tmds
commented
May 20, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs
Outdated

@runfoapprunfoappBot
mentioned this pull request
May 20, 2021

Closed

stephentoub
stephentoub
reviewed
May 25, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // Update the state if we've become connected after a non-blocking connect.
 _isConnected = true;
 _rightEndPoint = _nonBlockingConnectRightEndPoint;
 _rightEndPoint ??= _nonBlockingConnectRightEndPoint;
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In what situation will it be non-null? I'm wondering in what situation we previously would have overwritten it and now we won't...
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I didn't think about this much. I replaced everything with a conditional assignment except for the one in Bind.
It can be non-null if for example you Bind first, and then do a Connect and then call LocalEndPoint and end up in this branch.

We don't want to overwrite it because that will lose the tracking (via _rightEndPoint) of the unix socket file we need to delete.
@antonfirsovantonfirsovMay 25, 2021•

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.

Maybe it would make sense to add a test for this case, so we don't break it in the future. Scratch it would be too difficult because of timing.
antonfirsov
antonfirsov
reviewed
May 25, 2021
View reviewed changes
@antonfirsovantonfirsov
left a comment
•

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.

Looks good after fixing the test (https://github.com/dotnet/runtime/pull/52103/files#r638644278) and answering https://github.com/dotnet/runtime/pull/52103/files#r638385763 (missed the answer).
@antonfirsov
antonfirsov
commented
May 25, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
May 26, 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. CI failures are unrelated build failures.
@antonfirsov
antonfirsov
merged commit d3ed5a9
into
dotnet:mainMay 31, 2021
@tmdstmds
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Sockets

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

proposal: delete unix socket path on Dispose of bound Socket


6 participants

@tmds@wfurt@antonfirsov@karelz@stephentoub@geoffkizer

, '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
Socket: delete unix local endpoint filename on Close - #52103
Merged
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
May 31, 2021
Merged
Socket: delete unix local endpoint filename on Close#52103
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
Conversation
@tmds
@tmdstmds
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
@ghost
ghost
commented
Apr 30, 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
Fixes #45537
@antonfirsov@geoffkizer ptal
Author:tmds
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@tmds
tmds
commented
Apr 30, 2021

Copy link


Copy Markdown

MemberAuthor
I just realized relative paths need specific handling. The working directory may change between the time of Bind and Dispose. I'll update the PR for this, no need to review it yet.
@wfurt
wfurt
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@tmds
tmds
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
The file is created when the socket is bound. If the file is already there, the socket will fail to bind.
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // correct type (IPEndPoint, etc). The Bind operation sets _rightEndPoint. Other operations must only set
 // it when the value is still null.
 // This enables tracking the file created by UnixDomainSocketEndPoint when the Socket is bound,
 // and to delete that file when the Socket gets disposed.
@tmdstmdsMay 3, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I avoided adding a field to Socket for the stake of tracking the file to be deleted. Instead this is tracked through _rightEndPointUnixDomainSocketEndPoint.BoundFileName.

On Bind an instance is created that has this property set. And on dispose, the file then gets deleted.
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've wondered this for a while... what does the "right" part of the name mean?
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Just guessing: right type. The name is not very clear. Maybe we should change it to _typedEndpoint or _endpointType?
@antonfirsovantonfirsovMay 25, 2021

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.

I keep asking this question every time I see that variable.
@geoffkizergeoffkizerMay 25, 2021

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.

I don't really know either. Would love to understand.
It might be "right" in the sense that it's the result after resolving wildcards like port=0. Does that seem right?
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated

tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
@tmds
tmds
commented
May 11, 2021

Copy link


Copy Markdown

MemberAuthor
@antonfirsov@geoffkizer can you continue reviewing this?
@antonfirsov
antonfirsov
commented
May 11, 2021

Copy link


Copy Markdown

Contributor
@tmds the UnixDomainSocketEndPoint changes look good to me. I'm only concerned about caching optimizations (discussion above).
@karelz
karelz
commented
May 18, 2021

Copy link


Copy Markdown

Member
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
@tmds
tmds
commented
May 18, 2021

Copy link


Copy Markdown

MemberAuthor
I need to look at the CI failures, they seem related to the change.
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
The discussion is more about #39313 than it is about this PR. Can we remove the optimization if it is decided to revert that PR? It will then happen as part of removing the _localEndPoint field that was introduced in #39313. @antonfirsov wdyt?
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

Contributor
@tmds we are undecided yet re reverting _localEndpoint I plan to open a separate issue to continue discussions on that topic. The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
I need to look at the CI failures, they seem related to the change.
I can't see the failures, I assume it was outerloop which disappeared from the log, will rerun those tests.
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@tmds
tmds
commented
May 19, 2021

Copy link


Copy Markdown

MemberAuthor
The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
The concern isn't about how this PR uses _localEndpoint but whether there should be a _localEndpoint. I don't see why I shouldn't use it while it is here, but I'll remove it based on your feedback.
tmds
tmds
commented
May 20, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs
Outdated

@runfoapprunfoappBot
mentioned this pull request
May 20, 2021

Closed

stephentoub
stephentoub
reviewed
May 25, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // Update the state if we've become connected after a non-blocking connect.
 _isConnected = true;
 _rightEndPoint = _nonBlockingConnectRightEndPoint;
 _rightEndPoint ??= _nonBlockingConnectRightEndPoint;
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In what situation will it be non-null? I'm wondering in what situation we previously would have overwritten it and now we won't...
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I didn't think about this much. I replaced everything with a conditional assignment except for the one in Bind.
It can be non-null if for example you Bind first, and then do a Connect and then call LocalEndPoint and end up in this branch.

We don't want to overwrite it because that will lose the tracking (via _rightEndPoint) of the unix socket file we need to delete.
@antonfirsovantonfirsovMay 25, 2021•

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.

Maybe it would make sense to add a test for this case, so we don't break it in the future. Scratch it would be too difficult because of timing.
antonfirsov
antonfirsov
reviewed
May 25, 2021
View reviewed changes
@antonfirsovantonfirsov
left a comment
•

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.

Looks good after fixing the test (https://github.com/dotnet/runtime/pull/52103/files#r638644278) and answering https://github.com/dotnet/runtime/pull/52103/files#r638385763 (missed the answer).
@antonfirsov
antonfirsov
commented
May 25, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
May 26, 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. CI failures are unrelated build failures.
@antonfirsov
antonfirsov
merged commit d3ed5a9
into
dotnet:mainMay 31, 2021
@tmdstmds
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Sockets

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

proposal: delete unix socket path on Dispose of bound Socket


6 participants

@tmds@wfurt@antonfirsov@karelz@stephentoub@geoffkizer

, '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
Socket: delete unix local endpoint filename on Close - #52103
Merged
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
May 31, 2021
Merged
Socket: delete unix local endpoint filename on Close#52103
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
Conversation
@tmds
@tmdstmds
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
@ghost
ghost
commented
Apr 30, 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
Fixes #45537
@antonfirsov@geoffkizer ptal
Author:tmds
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@tmds
tmds
commented
Apr 30, 2021

Copy link


Copy Markdown

MemberAuthor
I just realized relative paths need specific handling. The working directory may change between the time of Bind and Dispose. I'll update the PR for this, no need to review it yet.
@wfurt
wfurt
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@tmds
tmds
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
The file is created when the socket is bound. If the file is already there, the socket will fail to bind.
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // correct type (IPEndPoint, etc). The Bind operation sets _rightEndPoint. Other operations must only set
 // it when the value is still null.
 // This enables tracking the file created by UnixDomainSocketEndPoint when the Socket is bound,
 // and to delete that file when the Socket gets disposed.
@tmdstmdsMay 3, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I avoided adding a field to Socket for the stake of tracking the file to be deleted. Instead this is tracked through _rightEndPointUnixDomainSocketEndPoint.BoundFileName.

On Bind an instance is created that has this property set. And on dispose, the file then gets deleted.
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've wondered this for a while... what does the "right" part of the name mean?
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Just guessing: right type. The name is not very clear. Maybe we should change it to _typedEndpoint or _endpointType?
@antonfirsovantonfirsovMay 25, 2021

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.

I keep asking this question every time I see that variable.
@geoffkizergeoffkizerMay 25, 2021

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.

I don't really know either. Would love to understand.
It might be "right" in the sense that it's the result after resolving wildcards like port=0. Does that seem right?
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated

tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
@tmds
tmds
commented
May 11, 2021

Copy link


Copy Markdown

MemberAuthor
@antonfirsov@geoffkizer can you continue reviewing this?
@antonfirsov
antonfirsov
commented
May 11, 2021

Copy link


Copy Markdown

Contributor
@tmds the UnixDomainSocketEndPoint changes look good to me. I'm only concerned about caching optimizations (discussion above).
@karelz
karelz
commented
May 18, 2021

Copy link


Copy Markdown

Member
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
@tmds
tmds
commented
May 18, 2021

Copy link


Copy Markdown

MemberAuthor
I need to look at the CI failures, they seem related to the change.
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
The discussion is more about #39313 than it is about this PR. Can we remove the optimization if it is decided to revert that PR? It will then happen as part of removing the _localEndPoint field that was introduced in #39313. @antonfirsov wdyt?
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

Contributor
@tmds we are undecided yet re reverting _localEndpoint I plan to open a separate issue to continue discussions on that topic. The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
I need to look at the CI failures, they seem related to the change.
I can't see the failures, I assume it was outerloop which disappeared from the log, will rerun those tests.
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@tmds
tmds
commented
May 19, 2021

Copy link


Copy Markdown

MemberAuthor
The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
The concern isn't about how this PR uses _localEndpoint but whether there should be a _localEndpoint. I don't see why I shouldn't use it while it is here, but I'll remove it based on your feedback.
tmds
tmds
commented
May 20, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs
Outdated

@runfoapprunfoappBot
mentioned this pull request
May 20, 2021

Closed

stephentoub
stephentoub
reviewed
May 25, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // Update the state if we've become connected after a non-blocking connect.
 _isConnected = true;
 _rightEndPoint = _nonBlockingConnectRightEndPoint;
 _rightEndPoint ??= _nonBlockingConnectRightEndPoint;
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In what situation will it be non-null? I'm wondering in what situation we previously would have overwritten it and now we won't...
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I didn't think about this much. I replaced everything with a conditional assignment except for the one in Bind.
It can be non-null if for example you Bind first, and then do a Connect and then call LocalEndPoint and end up in this branch.

We don't want to overwrite it because that will lose the tracking (via _rightEndPoint) of the unix socket file we need to delete.
@antonfirsovantonfirsovMay 25, 2021•

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.

Maybe it would make sense to add a test for this case, so we don't break it in the future. Scratch it would be too difficult because of timing.
antonfirsov
antonfirsov
reviewed
May 25, 2021
View reviewed changes
@antonfirsovantonfirsov
left a comment
•

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.

Looks good after fixing the test (https://github.com/dotnet/runtime/pull/52103/files#r638644278) and answering https://github.com/dotnet/runtime/pull/52103/files#r638385763 (missed the answer).
@antonfirsov
antonfirsov
commented
May 25, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
May 26, 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. CI failures are unrelated build failures.
@antonfirsov
antonfirsov
merged commit d3ed5a9
into
dotnet:mainMay 31, 2021
@tmdstmds
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Sockets

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

proposal: delete unix socket path on Dispose of bound Socket


6 participants

@tmds@wfurt@antonfirsov@karelz@stephentoub@geoffkizer

, '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
Socket: delete unix local endpoint filename on Close - #52103
Merged
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
May 31, 2021
Merged
Socket: delete unix local endpoint filename on Close#52103
antonfirsov merged 13 commits into
dotnet:mainfrom 
tmds:unix_socket_delete_file
Conversation
@tmds
@tmdstmds
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
@ghost
ghost
commented
Apr 30, 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
Fixes #45537
@antonfirsov@geoffkizer ptal
Author:tmds
Assignees:-
Labels:
area-System.Net.Sockets
Milestone:-
@tmds
tmds
commented
Apr 30, 2021

Copy link


Copy Markdown

MemberAuthor
I just realized relative paths need specific handling. The working directory may change between the time of Bind and Dispose. I'll update the PR for this, no need to review it yet.
@wfurt
wfurt
commented
Apr 30, 2021

Copy link


Copy Markdown

Member
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
@antonfirsov

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@tmds
tmds
commented
May 3, 2021

Copy link


Copy Markdown

MemberAuthor
should the file be deleted only if created by .NET ? ... or is it always the case e.g. the local endpoint is transient?
The file is created when the socket is bound. If the file is already there, the socket will fail to bind.
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // correct type (IPEndPoint, etc). The Bind operation sets _rightEndPoint. Other operations must only set
 // it when the value is still null.
 // This enables tracking the file created by UnixDomainSocketEndPoint when the Socket is bound,
 // and to delete that file when the Socket gets disposed.
@tmdstmdsMay 3, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I avoided adding a field to Socket for the stake of tracking the file to be deleted. Instead this is tracked through _rightEndPointUnixDomainSocketEndPoint.BoundFileName.

On Bind an instance is created that has this property set. And on dispose, the file then gets deleted.
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I've wondered this for a while... what does the "right" part of the name mean?
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Just guessing: right type. The name is not very clear. Maybe we should change it to _typedEndpoint or _endpointType?
@antonfirsovantonfirsovMay 25, 2021

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.

I keep asking this question every time I see that variable.
@geoffkizergeoffkizerMay 25, 2021

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.

I don't really know either. Would love to understand.
It might be "right" in the sense that it's the result after resolving wildcards like port=0. Does that seem right?
tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
Outdated

tmds
tmds
commented
May 3, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
@tmds
tmds
commented
May 11, 2021

Copy link


Copy Markdown

MemberAuthor
@antonfirsov@geoffkizer can you continue reviewing this?
@antonfirsov
antonfirsov
commented
May 11, 2021

Copy link


Copy Markdown

Contributor
@tmds the UnixDomainSocketEndPoint changes look good to me. I'm only concerned about caching optimizations (discussion above).
@karelz
karelz
commented
May 18, 2021

Copy link


Copy Markdown

Member
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
@tmds
tmds
commented
May 18, 2021

Copy link


Copy Markdown

MemberAuthor
I need to look at the CI failures, they seem related to the change.
Triage: @tmds can you please remove endpoint optimization from this change? We can make it separate PR and discuss there if and how we want it to behave.
The discussion is more about #39313 than it is about this PR. Can we remove the optimization if it is decided to revert that PR? It will then happen as part of removing the _localEndPoint field that was introduced in #39313. @antonfirsov wdyt?
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

Contributor
@tmds we are undecided yet re reverting _localEndpoint I plan to open a separate issue to continue discussions on that topic. The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
I need to look at the CI failures, they seem related to the change.
I can't see the failures, I assume it was outerloop which disappeared from the log, will rerun those tests.
@antonfirsov
antonfirsov
commented
May 19, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
@tmds
tmds
commented
May 19, 2021

Copy link


Copy Markdown

MemberAuthor
The point is that in this PR it is better to push this concern out of scope and revert the changes dealing with _localEndpoint, so this question does not block us to go ahead and merge the UDS improvement.
The concern isn't about how this PR uses _localEndpoint but whether there should be a _localEndpoint. I don't see why I shouldn't use it while it is here, but I'll remove it based on your feedback.
tmds
tmds
commented
May 20, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs
Outdated

@runfoapprunfoappBot
mentioned this pull request
May 20, 2021

Closed

stephentoub
stephentoub
reviewed
May 25, 2021
View reviewed changes
Comment threadsrc/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
 // Update the state if we've become connected after a non-blocking connect.
 _isConnected = true;
 _rightEndPoint = _nonBlockingConnectRightEndPoint;
 _rightEndPoint ??= _nonBlockingConnectRightEndPoint;
@stephentoubstephentoubMay 25, 2021

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

In what situation will it be non-null? I'm wondering in what situation we previously would have overwritten it and now we won't...
@tmdstmdsMay 25, 2021

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

I didn't think about this much. I replaced everything with a conditional assignment except for the one in Bind.
It can be non-null if for example you Bind first, and then do a Connect and then call LocalEndPoint and end up in this branch.

We don't want to overwrite it because that will lose the tracking (via _rightEndPoint) of the unix socket file we need to delete.
@antonfirsovantonfirsovMay 25, 2021•

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.

Maybe it would make sense to add a test for this case, so we don't break it in the future. Scratch it would be too difficult because of timing.
antonfirsov
antonfirsov
reviewed
May 25, 2021
View reviewed changes
@antonfirsovantonfirsov
left a comment
•

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.

Looks good after fixing the test (https://github.com/dotnet/runtime/pull/52103/files#r638644278) and answering https://github.com/dotnet/runtime/pull/52103/files#r638385763 (missed the answer).
@antonfirsov
antonfirsov
commented
May 25, 2021

Copy link


Copy Markdown

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

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
antonfirsov
antonfirsov
approved these changes
May 26, 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. CI failures are unrelated build failures.
@antonfirsov
antonfirsov
merged commit d3ed5a9
into
dotnet:mainMay 31, 2021
@tmdstmds
mentioned this pull request
May 31, 2021

Closed

thaystg added a commit
to thaystg/runtime
that referenced
this pull request
Jun 1, 2021
@thaystg
Merge remote-tracking branch 'upstream/main' into thays_remove_mini_w…
c4bf8fc
…asm_debugger_and_use_debugger_agent
* upstream/main: (597 commits)
Fix42292 (dotnet#52463)
[mono] Remove some obsolete emscripten flags. (dotnet#53486)
Fixed path to projects (dotnet#53435)
support ServerCertificateContext in quic (dotnet#53175)
Socket: delete unix local endpoint filename on Close (dotnet#52103)
[mono] Fix sgen_gc_info.memory_load_bytes (dotnet#53364)
Refactor MsQuic's native IP address types. (dotnet#53461)
Re-enabled optimizations for gtFoldExprConst (dotnet#53347)
Add diagnostic support into sample app and AppBuilders on Mono. (dotnet#53361)
Fix issues with virtuals and mdarrays (dotnet#53400)
Split Variant marshalling tests (dotnet#53035)
Update clrjit.natvis to cover GT_SIMD and GT_HWINTRINSIC (dotnet#53470)
remove WSL checks in tests (dotnet#53475)
Always spawn message loop thread for SystemEvents (dotnet#53467)
add AcceptAsync cancellation overloads (dotnet#53340)
Remove unnecessary reference to iOS workload pack in the Mono workload (dotnet#53425)
Add CookieContainer.GetAllCookies (dotnet#53441)
Remove DynamicallyAccessedMembers on JsonSerializer (dotnet#53235)
[wasm] Re-enable Wasm.Build.Tests (dotnet#53433)
[libraries] Move library tests Feature Switches defaults to Functional tests (dotnet#53253)
...
@ghostghost
locked as resolved and limited conversation to collaborators
Jun 30, 2021
@karelzkarelz
added this to the 6.0.0 milestone
Jul 15, 2021
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@stephentoubstephentoubstephentoub left review comments

+2 more reviewers

@geoffkizergeoffkizergeoffkizer left review comments
@antonfirsovantonfirsovantonfirsov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Net.Sockets

Projects


None yet


Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

proposal: delete unix socket path on Dispose of bound Socket


6 participants

@tmds@wfurt@antonfirsov@karelz@stephentoub@geoffkizer