Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@IlyaBiryukov
Copy link
Contributor

With code separation into System.Net.Primitives and System.Net.Sockets,
EndPoint extensibility was broken because System.Net.Sockets started to
use its own copy of SocketAddress and didn't respect SocketAddress that
a custom EndPoint may provide.

The fix is to allow conversion between SocketAddress from System.Net.Primitives
and System.Net.Sockets. This way custom implementations of EndPoint will
be able to provide their own SocketAddress and it'll be honored by the
Socket APIs.

The fix also allows sockets to use 'Unspecified' protocol type which is
needed for Unix Domain Sockets. There are several changes in socket test
server to allow tests pass protocol type.

Add new unit tests that use end point extensibility to implement Unix
Domain Sockets.

Fix #4777

@dnfclas
Copy link

dnfclas commented Dec 17, 2015

Hi @IlyaBiryukov, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@IlyaBiryukov
Copy link
Contributor Author

I've just associated my GitHub account with Microsoft before creating this pull request, I guess the DNFBOT doesn't know that yet and added cla-required tag.

@davidsh
Copy link
Contributor

davidsh commented Dec 17, 2015

Even if you associate your GitHub account with your Microsoft account, you still need to sign the CLA.

@davidsh
Copy link
Contributor

davidsh commented Dec 17, 2015

cc: @CIPop @pgavlin

@IlyaBiryukov
Copy link
Contributor Author

cc: @AArnott

@Maxwe11
Copy link
Contributor

Maxwe11 commented Dec 17, 2015

I've just associated my GitHub account with Microsoft before creating this pull request, I guess the DNFBOT doesn't know that yet and added cla-required tag.

Commit signed off by @ilyab which is not associated with Microsoft 😄

@IlyaBiryukov
Copy link
Contributor Author

Fixed the commit author.

@dnfclas
Copy link

dnfclas commented Dec 17, 2015

@IlyaBiryukov, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

Copy link
Contributor

Choose a reason for hiding this comment

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

What version of OS X are you running? On 10.9, the max path length is 104: https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man4/unix.4.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

108 is on Linux. OSX has 104. So we should make it 104 I guess..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docs say that it may even be as less as 92, for example here, http://man7.org/linux/man-pages/man7/unix.7.html. I'll change it to 92.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. FWIW, the Open Group Base Specification is generally a good source of truth for these things, and it agrees with 92 as a lower bound.

@AArnott
Copy link

AArnott commented Dec 18, 2015

LGTM

@IlyaBiryukov
Copy link
Contributor Author

If there are no more comments, how can this pull request be committed?

Copy link

Choose a reason for hiding this comment

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

How would this work for a DnsEndPoint? Please add a test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are already existing tests for DnsEndpoint. The code checks if it's DNS end point before it calls IPEndPointExtensions.

@IlyaBiryukov
Copy link
Contributor Author

I addressed all the comments. Is there anything else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: we might want this to be in a finally block to ensure that we run cleanup even if the test fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

@pgavlin
Copy link
Contributor

pgavlin commented Dec 22, 2015

LGTM aside from one nit. Thanks for doing this!

With code separation into System.Net.Primitives and System.Net.Sockets,
EndPoint extensibility was broken because System.Net.Sockets started to
use its own copy of SocketAddress and didn't respect SocketAddress that
a custom EndPoint may provide.

The fix is to allow conversion between SocketAddress from System.Net.Primitives
and System.Net.Sockets. This way custom implementations of EndPoint will
be able to provide their own SocketAddress and it'll be honored by the
Socket APIs.

The fix also allows sockets to use 'Unspecified' protocol type which is
needed for Unix Domain Sockets. There are several changes in socket test
server to allow tests pass protocol type.

Add new unit tests that use end point extensibility to implement Unix
Domain Sockets.

Fix #4777
pgavlin added a commit that referenced this pull request Dec 22, 2015
Enable custom socket end points and allow Unix Domain Sockets
@pgavlin pgavlin merged commit 895cdac into dotnet:master Dec 22, 2015
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Net.Socket EndPoint-extensibility broken in corefx

8 participants