Require <S: Read + Write> instead of TcpStream

[sciguy16]

Hi, I would like to be able to connect to VNC servers via a SOCKS proxy, but the Client::from_tcp_stream method requires a TcpStream. If this is changed to a generic type that implements Read+Write then it will be compatible with other types of streams, like Socks5Stream from the 'socks' crate.

https://github.com/whitequark/rust-vnc/blob/259ea4ce1e895125788c64017a97108584a78791/src/client.rs#L167

I'm happy to contribute a PR for this, and also to update the code for Rust 2018 if you'd like.

[whitequark]

The switch to a generic type sounds good. It's API-breaking, but that's a reasonable reason to break things.

[sciguy16]

Slight problem with Read + Write: https://github.com/whitequark/rust-vnc/blob/259ea4ce1e895125788c64017a97108584a78791/src/client.rs#L294-L302

I don't think there's a trait we can require for try_clone, and TcpStream doesn't implement Clone, so I wonder whether it might be reasonable to wrap the stream in an Arc or a Mutex and clone the reference to that instead.

[whitequark]

That's kind of disappointing to have to do that, what do other libraries do?

[sciguy16]

The only other crate I've really looked at for this is rdp-rs which doesn't spawn any extra threads so it's not a problem. There's a dead PR in Rust for a TryClone trait which would have been useful here.

I'm not sure there's going to be a good way to duplicate the handle or split reading and writing. An alternative could be to refactor all the writes into the Event::pump thread with another mpsc channel, but that feels clunky.

[whitequark]

Ah right rust-vnc uses a worker thread. Let's do Arc then.

[sciguy16]

Sounds like a plan. I've already updated the codebase to 2018 - would you like it in a separate PR or all together?

[sciguy16]

Also, calling shutdown() won't work on a Read + Write, but it should be closed properly when it's dropped so removing the method shouldn't cause any problems beyond API breakage.

https://github.com/whitequark/rust-vnc/blob/259ea4ce1e895125788c64017a97108584a78791/src/client.rs#L420-L423

[whitequark]

I've already updated the codebase to 2018 - would you like it in a separate PR or all together?

Separate

[sciguy16]

Using a mutex to share the stream isn't going well. Without access to the raw file descriptor the event poll thread has no way of knowing whether there's data available and just blocks forever. It might be possible to rework the code slightly to work with nonblocking sockets, but that adds an assumption about the socket that we can't check or control for.

An alternative could be to expose an async version that uses AsyncRead + AsyncWrite that would be compatible with tokio-socks, although this would probably have the same deadlock issue as it would need to keep the mutex locked while await-ing it.

A Tokio TcpStream can be split into read- and write halves that can be passed to separate threads, but the tokio-socks Socks5Stream doesn't look like it can do that.

I think the main blocker here is the socks crates haven't really been designed to be accessed from multiple threads.

[whitequark]

Seems like it should be fixed in the socks crate, then?

[sciguy16]

Yeah, I'll probably have to look at the socks implementations and come back to this issue later

[HsuJv]

Hi all here,

I've got the same scenario that needs <S: Read + Write> in my app (In fact is WebSocket). And found this issue seems inactive for quite a long time so I wrote another crate to resolve this issue. Hope it can be helpful to you.

BRs.