Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider exposing Socket(SafeSocketHandle fd) constructor #862

Closed
dmirmilshteyn opened this issue Nov 27, 2016 · 28 comments · Fixed by #34727
Closed

Consider exposing Socket(SafeSocketHandle fd) constructor #862

dmirmilshteyn opened this issue Nov 27, 2016 · 28 comments · Fixed by #34727
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Milestone

Comments

@dmirmilshteyn
Copy link

dmirmilshteyn commented Nov 27, 2016


EDIT: Revised proposal by @stephentoub on 4/3/2020:

public Socket(SafeSocketHandle handle);

System.Net.Sockets.Socket currently does not expose any mechanism to instantiate a socket from an existing file descriptor/handle. This proposal aims to expose this functionality to improve interop with external applications that manage socket connections.

Rationale and Usage

Currently, a socket must be both created and managed by the user application. While this is desired behaviour in most cases, there is no mechanism to take an existing socket handle/file descriptor and make use of it within the user application. If one wanted to use a socket that was created by an external application, the only option (outside of reflection hacks) is to reimplement the calls to the underlying platform sockets with P/Invoke - largely removing the benefits that the managed socket classes provide.

More specifically, this improves interop with existing applications that manage socket connections, such as systemd and the socket activation feature. In this scenario, systemd creates an unmanaged socket to listen for connections. Once a connection is established, a .NET application is launched and a file descriptor that references the unmanaged socked is passed to it through environment variables. Without the ability to create a managed socket from a file descriptor, the connection will need to be reset and then reestablished using the managed APIs - rather than cleanly being passed through and maintained.

Proposed API

namespace System.Net.Sockets
{
    // Previously implemented socket class
    public partial class Socket : System.IDisposable
    {
        // Existing constructors
        public Socket(System.Net.Sockets.AddressFamily addressFamily, System.Net.Sockets.SocketType socketType, System.Net.Sockets.ProtocolType protocolType) { }
        public Socket(System.Net.Sockets.SocketInformation socketInformation) { }
        public Socket(System.Net.Sockets.SocketType socketType, System.Net.Sockets.ProtocolType protocolType) { }   
        private Socket(SafeCloseSocket fd) { }

        // Additional existing members omitted

        // Proposed member
        public Socket(IntPtr fileDescriptor, AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType) { }
    }
}

Details

  • Adding this rather than exposing the existing Socket(SafeCloseSocket fd) constructor will keep the rest of the socket PAL internal, as there's currently public no way (as far as I can tell) to create an instance of SafeCloseSocket from a IntPtr.
  • This will align with user expectations coming from other languages, such as Python, which has a similar method to create a socket from a file descriptor (see socket.fromfd(fd, family, type[, proto])).
  • Platforms that don't support wrapping unmanaged sockets (or that don't expose them, such as UWP?) will throw PlatformNotSupportedException.
@davidsh
Copy link
Contributor

davidsh commented Feb 28, 2017

Please take a look at:
https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/adding-api-guidelines.md

for API change process and submit API proposal.

@geoffkizer
Copy link
Contributor

I agree, this would be a good feature to have.

@karelz
Copy link
Member

karelz commented Mar 4, 2017

Next step: We need formal API proposal (linked above)

@dmirmilshteyn
Copy link
Author

@karelz Here's the proposal. This is my first contribution here, so please let me know if I need to add more details/adjustments.

Proposal: Add a new constructor to System.Net.Sockets.Socket that allows instantiating a socket from a file descriptor. I imagine this would also need to be added to the next netstandard version, but this shouldn't introduce any extra dependencies/other breaking API changes.

public Socket(IntPtr fileDescriptor, AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType)

Adding this rather than exposing the existing Socket(SafeCloseSocket fd) constructor will keep the rest of the socket PAL internal, as there's currently public no way (as far as I can tell) to create an instance of SafeCloseSocket from a IntPtr. This will also align with user expectations coming from other languages, such as Python, which has a similar method to create a socket from a file descriptor (see socket.fromfd).

Use cases: support features where external programs manage socket connections (such as systemd socket activation), improved interop with native code and existing unix tools

@karelz
Copy link
Member

karelz commented Mar 4, 2017

Can you please use the API template in API process? It's much, much easier to read ;) Thanks!
Also please copy/move it into the top post - it is easier to find the latest version of the proposal.

@geoffkizer
Copy link
Contributor

@stephentoub Thoughts on this?

@dmirmilshteyn
Copy link
Author

@karelz I've updated the top post based on the example from that document.

@karelz
Copy link
Member

karelz commented Mar 4, 2017

Great, thank you!

Does the fileDescriptor always match OS bitness on all OSs - Windows, Linux and Mac OS APIs?

@davidsh @geoffkizer @CIPop @Priya91 please comment or give thumbs up, thanks!

@stephentoub
Copy link
Member

Does the fileDescriptor always match OS bitness on all OSs - Windows, Linux and Mac OS APIs?

file descriptors on Unix are <= 32K. Since handles in Windows are represented as pointers, we've been using IntPtr everywhere in corefx to represent both, which allows us to use SafeHandle for them.

@stephentoub Thoughts on this?

Seems like an ok API to expose.

One modification though:
I would suggest that the ctor take a SafeHandle rather than a raw IntPtr. The plan for https://github.com/dotnet/corefx/issues/13470, which has been approved but not yet implemented, is to expose a SafeSocketHandle (probably just rename SafeCloseSocket). Taking a SafeHandle like this will make ownership clearer. Just taking an IntPtr, is the socket responsible for closing the socket when Socket is disposed or not? So you'd also need to take an ownsHandle flag or something similar, and the Socket will end up creating a SafeSocketHandle anyway under the covers.

Related to this, I also wonder if there are implicit assumptions in Socket's implementation related to lifetime. I expect some vetting of the implementation will need to be done and it won't be quite as simple as just adding a ctor, though it may turn out to be all the actual code that's needed.

@tmds
Copy link
Member

tmds commented Jul 4, 2017

Perhaps also consider a public Socket(SafeCloseSocket)?

On Linux, the address family, socket type and protocol can be retrieved as SO_DOMAIN, SO_PROTOCOL, SO_TYPE. SO_TYPE is POSIX, the other two are not.
When the OS does not support retrieving the values, they can be set to the enums Unknown value.

@orthoxerox
Copy link

👍 to this. I've been looking for a way to restart an application without losing open TCP connections, and while Process.Start() ensures the child process inherits all the handles, it cannot access the open sockets.

@Tragetaschen
Copy link
Contributor

I've got my DBus client and get a file descriptor from BlueZ representing the RFCOMM socket when a smart phone connects to a registered Bluetooth profile. I would love to work with this using all the socket goodness instead of wrapping it in a Stream and providing the bare minimum read and write pinvokes by myself.

@stephentoub
Copy link
Member

cc: @wfurt

@davidsh
Copy link
Contributor

davidsh commented May 2, 2019

We should do this as well: dotnet/corefx#26193

@mjsabby
Copy link
Contributor

mjsabby commented Jul 1, 2019

Are we waiting for an implementation or there is more to be discussed here?

If I'm not mistaken, the windows story here is already complete I believe, due to SocketInformation, but a cross-plat story is missing.

This would then be useful to implement a http.sys like port sharing behavior cross platform in Kestrel (albeit a user-mode multi-process one).

@jormenjanssen
Copy link

@krwq @stephentoub I tried the new enums from commit (dotnet/corefx#37315) in .Net Core 3 preview 7 with various combinations but it look's like CAN (Controller-Area-Network) is not properly implemented (At least i couldn't get it to work).

I implemented a own solution as temporary workaround to get the canbus interface to work in .Net Core. Maybe you guys could expose the interface for creating a raw socket or implement this properly al the way down (this is the preffered way).

You could find my full functional example/demo for sending and receive CAN message out-of-the-box in the following repo: https://github.com/jormenjanssen/netcore-can-example

@stephentoub
Copy link
Member

it look's like CAN (Controller-Area-Network) is not properly implemented (At least i couldn't get it to work).

cc: @wfurt

@wfurt
Copy link
Member

wfurt commented Aug 1, 2019

Far CAN, only RAW type is supported at the moment.
@krwq was doing some testing there. I would recommend to do strace for working and not working case and open separate specific issue. This one is about API change and we should not pollute it with CAN low level CAN issues.

@wfurt
Copy link
Member

wfurt commented Aug 1, 2019

As far is the main thread: It seems like we are either looking for public Socket(SafeCloseSocket) and we would try to determine type via some lookup functions or public Socket(SafeCloseSocket, AddressFamily addressFamily, SocketType socketType, ProtocolType protocolType)

It seems like we don't really need socketType and protocolType after constructor. One can always use ProtocolType.Unknown and SocketType.Unknown if that is not applicable. For the Unknown I'm not sure how that would work with DynamicWinsockMethods. Maybe somebody can try AF_BTH as example to make sure it works.

@krwq
Copy link
Member

krwq commented Aug 1, 2019

@jormenjanssen also for CAN you must also enable it on the device before usage externally (i.e. you must set speed etc) - you can use https://github.com/dotnet/iot/blob/master/src/devices/SocketCan/README.md to see what is exactly needed.

@stephentoub stephentoub changed the title Consider exposing Socket(SafeCloseSocket fd) constructor Consider exposing Socket(SafeSocketHandle fd) constructor Oct 4, 2019
@scalablecory
Copy link
Contributor

scalablecory commented Oct 22, 2019

Triage: we think the API is fine but don't want to go review it until we understand if it's worth the complexity to implement it. @wfurt can you please take a look at this, there are some interesting hurdles re: managing state for socket options, address family for udp, etc.

@maryamariyan maryamariyan transferred this issue from dotnet/corefx Dec 13, 2019
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Net.Sockets untriaged New issue has not been triaged by the area owner labels Dec 13, 2019
@maryamariyan maryamariyan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Dec 13, 2019
@maryamariyan maryamariyan added this to the 5.0 milestone Dec 13, 2019
@scalablecory
Copy link
Contributor

We have two internal partners asking for this. CC @samsp-msft

@antonfirsov antonfirsov removed the untriaged New issue has not been triaged by the area owner label Jan 15, 2020
@stephentoub stephentoub self-assigned this Apr 1, 2020
@stephentoub stephentoub added api-ready-for-review and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 3, 2020
@wfurt
Copy link
Member

wfurt commented Apr 3, 2020

SO_DOMAIN and SO_PROTOCOL is not POSIX but it does exist and work on FreeBSD. Windows have SO_PROTOCOL_INFO. With that we should be able to reconstruct following properties:

AddressFamily
Blocking
Connected
IsBound
LocalEndPoint
ProtocolType
RemoteEndPoint
SocketType

That leaves macOS. When socket is bound or connected we can get AddressFamily from the address. Also getsockopt(fd, IPPROTO_IP, *) fails with EINVAL on PF_INET6 socket. It should take up to 3 probes to determine there families e.g. InterNetwork, InterNetwork6 and Unix .

I did also some testing with using unsupported socket type on Linux e.g. Netlink. So far everything works with exception of Bind() and custom EndPoint. OS bind() still works and it can be used as workaround.

@stephentoub
Copy link
Member

With that we should be able to reconstruct following properties

Thanks for the notes!

Mostly. I actually have this implemented locally and wired up with #34158 (which was what I actually wanted to solve). However, we can't fully reconstruct everything you listed. Blocking, for example. I don't know of any way to query that on Windows. And on Unix, we can query it, but that tells us the actual state of the file descriptor's nonblocking setting, whereas in the managed layer we maintain some shadow state to keep Socket.Blocking behaving the way the developer set it but be able to use non-blocking under the covers as part of the async implementation.

Even with some minor hiccups / inconsistencies, I think this is still worthwhile. The majority use case for this is taking a file descriptor / handle that didn't come from a managed Socket, and without this ctor, there's no good way to achieve it, and with the ctor, the cited inconsistencies should be at worst minor annoyances.

@wfurt
Copy link
Member

wfurt commented Apr 3, 2020

For the Blocking, the special handling would apply only with async, right? So for the newly created socket it should be OK to reflect OS Blocking IMHO since no Async was done so far. This could possibly create issue if somebody used Handle from existing socket thinking it is Blocking but it is not under the cover e.g. the issue is with sending, not creating Socket from handle.

For Windows, I'm wondering if we could always set it as blocking and document that or if it would be better to live with possible inconsistency. In some way caller should know and structure the code accordingly.

The easy way to fix if somebody hits is to set it explicitly after creation, right? If we see use case for creating Socket from existing Socket maybe we can add support for duplication/clone.

In either case, since access exist outside if the Socket, OS handle can be closed and that can also create surprises. Perhaps it would be worth of adding note about good practices.

@stephentoub
Copy link
Member

For the Blocking, the special handling would apply only with async, right?

Correct.

For Windows, I'm wondering if we could always set it as blocking

That's what I currently do. I don't see a better alternative.

The easy way to fix if somebody hits is to set it explicitly after creation, right?

Yes

@stephentoub
Copy link
Member

@wfurt, here's my branch, btw:
stephentoub@ffa06e1

@stephentoub stephentoub added the blocking Marks issues that we want to fast track in order to unblock other important work label Apr 7, 2020
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review blocking Marks issues that we want to fast track in order to unblock other important work labels Apr 7, 2020
@terrajobst
Copy link
Contributor

terrajobst commented Apr 7, 2020

Video

Looks good as proposed:

namespace System.Net.Sockets
{
    public partial class Socket
    {
        public Socket(SafeSocketHandle handle);
    }
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Sockets
Projects
None yet
Development

Successfully merging a pull request may close this issue.