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

Raw socket traits for Windows #859

Closed
wants to merge 2 commits into from
Closed

Raw socket traits for Windows #859

wants to merge 2 commits into from

Conversation

vikrrrr
Copy link

@vikrrrr vikrrrr commented Jul 22, 2018

This PR implements the {As,From,Into}RawSocket traits on TcpListener, TcpStream and UdpSocket for Windows.

This will allow for lower-level socket manipulation from outside of Mio on Windows (e.g. for TCP OOB streams). The step after this PR is to also implement those traits in Tokio.

@tobz
Copy link
Member

tobz commented Aug 11, 2018

@carllerche This looks pretty straight forward to me but I'm not sure how you feel about it?

@carllerche
Copy link
Member

I have been avoiding adding conversions from TcpStream -> windows raw sockets.

The reason is that Mio preemptively buffers data out of the TcpStream, i.e. Mio mucks with the socket state in ways that you might not expect. So, if you convert from TcpStream -> raw socket, you will probably lose data.

I'm not sure what the best way to deal with this is.

@carllerche
Copy link
Member

Can you clarify exactly what operations you are trying to do and what is needed?

Do you understand the current issue w/ Mio buffering internally?

@vikrrrr
Copy link
Author

vikrrrr commented Aug 29, 2018

A specific use-case is implementing the Telnet protocol correctly. As per IETF RFC 854, Telnet requires the use of TCP's urgent packets mechanism for its Synch signal (page 8-10). This mechanism is very often overlooked, very platform-specific and its use is error-prone. As such, it isn't and shouldn't be implemented in Mio itself. This requires allowing users to mess with raw sockets.

I am not familiar enough with Mio's code to truly understand the issue. Considering the traits implemented here are platform-specific and, to some extent, require the use of similarly platform-specific unsafe code to call Windows APIs, I think it would still be beneficial to implement these traits, as their users may be assumed to know their platforms' specificities. Now this raises the following question: how much control do users have over Mio's internal buffering and how much should they have? If this buffering is only a problem on certain platforms, maybe we should expose new platform-specific APIs to deal with these cases, if even possible.

@ustulation
Copy link
Contributor

Also a lot of protocols are implemented in other languages and they nearly all have APIs to accept raw sockets. E.g. UDT/SCTP-over-UDP etc. So say I use rust-mio-UDP to forge a NAT Traversal and after that I would want to give the ownership of the sockets to other libs (mostly in C) and provide a thin FFI to interact with them. This becomes impossible on Windows due to lack of conversion to raw sockets.

ustulation added a commit to maidsafe-archive/socket-collection that referenced this pull request Oct 14, 2018
Due to mio's lack of support for this, 'unimplement' this code on Windows.
See tokio-rs/mio#859 for more details.
Once that is merged, the function here can be implemented properly again.
@sdroege
Copy link

sdroege commented Dec 20, 2018

I also need this one here, specifically the AsRawSocket impl. The use case for me is that I want to handle the read and write side of an UDP socket independently, and for reasons beyond my control the read side is fortunately in tokio related code while the write side uses a different API that would only allow to have the socket passed there via a RawFd on UNIX and RawSocket on Windows. In my code I then use AsRawSocket to get a raw socket (or the equivalent with an fd), duplicate the socket (or fd) and pass the duplicated one to the other API.

That's all working fine on UNIX but due to missing API it doesn't on Windows.

Tokio-related PR is here tokio-rs/tokio#806


So in my case as well as @ustulation's case, the buffering does not really matter. But maybe to prevent potential problems of lost data, the IntoRawSocket trait should not be implemented here?

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

I would suggest to add some simple tests. For example checking if IntoRawSocket and AsRawSocket return equal values and a round trip test, i.e. calling IntoRawSocket and using that as input from FromRawSocket.

src/sys/windows/tcp.rs Outdated Show resolved Hide resolved
src/sys/windows/udp.rs Outdated Show resolved Hide resolved
unsafe fn from_raw_socket(socket: RawSocket) -> TcpStream {
TcpStream {
sys: FromRawSocket::from_raw_socket(socket),
selector_id: SelectorId::new(),
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw Dec 26, 2018

Choose a reason for hiding this comment

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

I'm not familiar with the Windows side of mio, but is it ok to create a new id here? It doesn't cause double events when calling TcpStream::from_raw_socket(stream.as_raw_socket()) TcpStream::from_raw_socket(stream.into_raw_socket())or something?

Copy link

Choose a reason for hiding this comment

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

If you do that you'd actually expect to get double events, or not? You have two mio sockets for the same underlying socket, and both should get events presumably.

It's probably not going to work very well though, actions on both sockets will likely trip over each other. AsRawSocket only really makes sense to be used to get a handle to the underlying socket to configure things on it, you have to be very careful to not break other users of the same socket as they usually assume unique ownership of it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sdroege I won't expect to get double events if I convert a TcpStream into a RawSocket, do some Windows' specific configuration and then convert it back into a TcpStream. Atleast on Unix this will work fine and won't generate two events, as only one file descriptor is open (and registered).

Copy link

Choose a reason for hiding this comment

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

Your example was using AsRawSocket, not IntoRawSocket. The former keeps the original mio socket alive, so I would expect to get events on both sockets. You'll have two mio sockets around the same underlying socket.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sdroege Sorry you're right I meant IntoRawSocket.

@sdroege
Copy link

sdroege commented Dec 27, 2018

I would suggest to add some simple tests. For example checking if IntoRawSocket and AsRawSocket return equal values and a round trip test, i.e. calling IntoRawSocket and using that as input from FromRawSocket.

That does not seem too useful though. The only way to have those tests ever fail is if one of those functions is creating a socket out of thin air instead (or WSADuplicateSocket the socket, which would be functionally equivalent to the correct behaviour) :)

@sdroege
Copy link

sdroege commented Dec 27, 2018

The PR generally looks fine to me, and should cover all the use-cases one might have but it would seem useful to put a big fat warning somewhere into the docs about IntoRawSocket due to the buffering that happens inside mio and that could cause data loss there.

@Thomasdezeeuw
Copy link
Collaborator

That does not seem too useful though. The only way to have those tests ever fail is if one of those functions is creating a socket out of thin air instead (or WSADuplicateSocket the socket, which would be functionally equivalent to the correct behaviour) :)

I get your point, however a new selector is also created, so it's not as simple as "packing"/"unpacking" the underlying file descriptor as what happens on Unix. I think its worth it to create a simple test for it, however if others disagree then feel free to merge it without a test.

@carllerche
Copy link
Member

Closing due to inactivity and the windows mio implementation is due for a rewrite which should enable this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants