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

API - Support Stream+Sink for ProxySocket? #1608

Open
ibigbug opened this issue Aug 11, 2024 · 11 comments
Open

API - Support Stream+Sink for ProxySocket? #1608

ibigbug opened this issue Aug 11, 2024 · 11 comments

Comments

@ibigbug
Copy link
Contributor

ibigbug commented Aug 11, 2024

The tcp stream does take opaque AsyncRead and AsyncWrite as underlying io https://docs.rs/shadowsocks/latest/shadowsocks/relay/tcprelay/proxy_stream/client/struct.ProxyClientStream.html#impl-AsyncRead-for-ProxyClientStream%3CS%3E

thoughts about having the udp socket to take Stream + Sink too ?

https://docs.rs/shadowsocks/latest/shadowsocks/relay/udprelay/proxy_socket/struct.ProxySocket.html

so it can be applied to proxy more general udp transports

@zonyitoo
Copy link
Collaborator

There is no Datagram based sockets in Rust's common libraries supports Read / Write or AsyncRead / AsyncWrite , here we are just following the common design in the Rust world.

so it can be applied to proxy more general udp transports

So what exactly problem are you struggling on?

@ibigbug
Copy link
Contributor Author

ibigbug commented Aug 12, 2024

there's Sink and Stream https://docs.rs/futures/latest/futures/sink/trait.Sink.html

i'd like to have ss wrap another UDP transport which impls Sink and Stream

@zonyitoo
Copy link
Collaborator

You can make a wrapper that wraps ProxyClientStream and make your own Sink

@ibigbug
Copy link
Contributor Author

ibigbug commented Aug 12, 2024

@ibigbug
Copy link
Contributor Author

ibigbug commented Aug 12, 2024

essentially i want to pass something that is Sink here https://github.com/Watfaq/clash-rs/blob/master/clash_lib/src/proxy/shadowsocks/mod.rs#L234

@zonyitoo
Copy link
Collaborator

zonyitoo commented Aug 12, 2024

So you want the underlying socket implements Sink and let ProxySocket::from_socket works with types that implemented Sink?

That would require quite a lot of changes in the current implementation.

  1. Let ShadowUdpSocket implements Sink
  2. Make all the APIs of ProxySocket works with interfaces provided by Stream and Sink.

I don't see the necessity here because the underlying connection is not a "stream" based connection.

@ibigbug
Copy link
Contributor Author

ibigbug commented Aug 12, 2024

There's nothing todo with a stream. and I think the Sink and Stream trait is the AsyncRead and AsyncWrite for Udp or any segmented io.

I know it's a fundamental change that requires amount of work.

I've had a look into the current code, it doesn't look like any udpsocket API is accessed other than the io related, which are fully replaceable by the Sink and Stream traits.

I'm happy to make the changes if it we agreed it's way to go

@ibigbug ibigbug changed the title API - Support Stream+Sync for ProxySocket? API - Support Stream+Sink for ProxySocket? Aug 13, 2024
@zonyitoo
Copy link
Collaborator

Please make a PR.

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 2, 2024

I'm working on this and realized a lot of the io methods take tokio ToSocketAddrs https://github.com/Watfaq/shadowsocks-rust/blob/7c154d2340849cdabb5971b3b6982f187a6b7efe/crates/shadowsocks-service/src/net/mon_socket.rs#L50

which is handing the DNS to sys resolver, should we change it to take plain SocketAddr and resolve the dns_resolver in the code base?

@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 3, 2024

ToSocketAddrs uses tokio's builtin DNS resolver, which is the libc's builtin getaddrinfo running in a thread-pool. In this project we use hickory-dns by default, which is built upon async/await framework.

@ibigbug
Copy link
Contributor Author

ibigbug commented Sep 3, 2024

Yeah I get it. Should we not use sys dns and use hickory dns for all?

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

No branches or pull requests

2 participants