Add peer_addr()

[ned14]

No description provided.

[meqif]

Thank you!

[ned14]

No problem. I've been looking into solutions for breaking UTP reads in a dedicated thread (really rust-lang/rust#23272). Would you have any objection to a FFI function which returns either when a socket has data to read or a timeout passes? I'd supply editions for both Windows and POSIX. I think this solution is nicer than sending magic UDP packets to break reads as it also allows a connection to timeout. Anyway, let me know and if you like it, I can rustle up some implementation code for you.

[dirvine]

Just as an FYI, have you guys seen https://github.com/libpnet/ which may be well suited to this issue. I know it's a dependency but if we maintain as much rust safety as possible it may be nice. Plus helps the language out.

[ned14]

@dirvine I wasn't aware that libpnet can do async or even non-blocking? It looks to be, from its docs, just as synchronous as Rust std::net.

[ned14]

Actually, Rust already provides the select() call, it's just not documented anywhere:

https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/c.rs
https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/c.rs

I'll try patching that into rust-utp and see what happens.

[meqif]

@ned14: I came across that issue after the IO reform. I also explored setting timeouts on the underlying UDP socket with the nix library in another branch (https://github.com/meqif/rust-utp/blob/timeout_on_receive/src/socket.rs#L338-L352), but got stuck on Windows support. If you could provide the Windows version, I would be grateful.

As the code stands right now, if one is using two threads to read and write (respectively) on a UtpCloneableSocket instance, there is no real blocking as long as there are no major disturbances (such as no new incoming packets for a while). If there are, it all comes down to the lack of timeout detection, which is necessary to send keep-alive packets, resend the current packet, or, ultimately, close the connection.

On the other hand, if there is only one thread controlling the socket, send_to will notice nobody else is consuming the incoming packets, and will recv as many packets as necessary to unblock itself if it's hitting the congestion window. This is a bit hacky, but it works quite well.

That was a long-winded way to say that the only problem I see right now is the lack of timeouts to send keep-alives and kill the connection. UtpCloneableSocket::recv_from blocks in the same way TcpStream::read does, making it possible to read data from it in a separate thread if ones wishes to do so.

@dirvine: Thanks, I saw libpnet a couple of weeks ago but nothing in its documentation suggested it was asynchronous.

[dirvine]

@ned14 @meqif Yes I jumped on their irc to clarify that and it is (as expected I suppose) a low level lib, so targeted at new protocols or network analysis tools (actually looking to use it in another crate for that). So sorry for noise.

[meqif]

@dirvine It was worth a try in any case. Thank you for checking.

[ned14]

@meqif: The problem is shutting down an existing transport ungracefully, so for example if crust has received a connection from someone and we decide that that connection is malicious, we want to drop it immediately and ungracefully. Same as a firewall. rust-utp, and indeed Rust in general, makes this hard for us, but crust currently uses shutdown() on TCP connections to force an early disconnect and there is no equivalent in rust-utp as close() appears to hang until the remote end sends a FIN packet. Hence right now crust hangs randomly during unit testing because it depends on the exact timing of when the reader writers are torn down. What we need instead is a force connection close which breaks any reads in any dedicated threads with a best effort graceful close, but falling back to ungraceful close quickly.

Does this make sense?

[ned14]

@meqif BTW on Windows if a read socket times out its SO_RCVTIMEO value, that socket becomes unusable after if I remember rightly. That probably explains your difficulties. Anyway, no problem with making a select() based timeout, that works everywhere.

[meqif]

@ned14: Oh, now I see. What I think would be best is having close try to end the connection gracefully for as long as the protocol mandates before giving up, and a shutdown method to end it forcefully with a reset packet (as per the UTP spec). Does this sound good to you?

As for Windows support, I didn't even get to the point of trying to implement it, as I got busier around that time and had to freeze my work on that branch. :)

[ned14]

@meqif Sounds great. Do you need a working socket timed read function still for that?

[meqif]

@ned14 Yes, a method to set a socket's read timeout in Windows (similarly to the set_read_timeout I wrote previously) would be great.

[ned14]

@meqif I can give you a replacement for UdpSocket::recv_from() which times out?

[dirvine]

To get through NAT devices we use hole punching, so this means longer than usual connect timeouts for those sockets (UDT actually used SO_ADDREUSE and swapped these for normal timeout sockets after connect). So a question here may be is the timeout related to connection establishment and if so is it possible to have these settable on socket instantiation or similar.
Not a huge deal right now I am just looking forward a wee bit and know this will be something to look at (called rendezvous connection in UDT) and maybe worthwhile adding to the thinking here.

[meqif]

@ned14 Wouldn't that imply a replacement for UdpSocket? A way to leverage this would be more than enough.

[meqif]

@dirvine The timeout used in the connect call is independent of a connected socket's "regular" timeout. I will leave that particular detail for a later improvement. :)

[ned14]

@meqif No replacement for UdpSocket needed. Such a timed_read() function might have function form timed_read(UdpSocket &, u32 timeout) -> Result<(bool, usize, SocketAddr)>.

Regarding https://github.com/rust-lang/rust/blob/master/src/libstd/sys/windows/net.rs#L134, that will only work for TCP sockets on Windows at best, and even then. I don't believe timeouts have worked with UDP sockets on Windows for a very long time now. In the end, all i/o on Windows is fundamentally async, and the sync APIs are emulation wrappers around the async APIs. Nobody doing serious programming on Windows uses any of the sync i/o APIs, they don't get much attention from Microsoft.

[meqif]

@ned14 Alright then, that sounds reasonable. What would be the meaning of the bool returned by the function?

[ned14]

@meqif Yeah that's not very Rust idiomatic is it! I was thinking of the bool as being true when a packet was read, else it timed out. I suppose an enum would be more idiomatic there? So let's say a Result<(usize, SocketAddr), CustomErrorType>? Or maybe a Result<Result<(usize, SocketAddr)>>? Whichever you prefer is fine with me.

[meqif]

@ned14 It looks like Result<(usize, SocketAddr)> with std::io::ErrorKind::TimedOut on timeout would be closer to what the standard library does (or used to).

[ned14]

@meqif That's perfect Ricardo. Thanks for the tip

[ned14]

@meqif I added a timed_read() function as per my description at ned14@83f8a14, but it currently panics the compiler. I'm out of work hours now, so I'll revisit on Monday and see if I can figure out what's upsetting the compiler.

[meqif]

@ned14 For what it's worth, it doesn't panic the compiler on OSX (rustc 1.2.0-nightly (20d23d8e5 2015-06-18)).

At a glance, I noticed you're missing a few imports (std::io::Result, and the use declarations need to be repeated inside mod test), and you won't be able to use socket.as_raw_fd() in timed_read, as that depends on a Unix-only trait (std::os::unix::io::AsRawFd, I believe).

The documentation of AsRawFd says that Windows has two corresponding traits, AsRawHandle and AsRawSocket, but they don't show up among the regular documentation, which doesn't bode well.

Aside from a few minor code style criticisms, it seems ok.

[meqif]

I was feeling restless, so I ended up implementing the timeout functionality myself in #8. Thanks for your code, @ned14, it was a great help.

Unfortunately, while the Unix version seems to work fine, the Windows version is not timing out as it should.

[ned14]

@meqif I can debug Windows for you tomorrow. Thanks for the speedy improvements.

[meqif]

@ned14 I got it working. :) I forgot to check if select returned 0, which should also be considered an error (ErrorKind::TimedOut, to be specific).