Matrix-Zhang / tokio_kcp

A Kcp implementation for tokio
MIT License
176 stars 44 forks source link

socket in struct KcpSocket should not be Arc<UdpSocket> #22

Closed aa51513 closed 1 year ago

aa51513 commented 1 year ago

As an alternative it should be socket: Arc

where S: AsyncRead + AsyncWrite + Unpin

After this change, it can be better used in other asynchronous frameworks

zonyitoo commented 1 year ago

The socket have to be shared between the polling task and the KcpSocket instance. So it has to be an Arc<>.

aa51513 commented 1 year ago

The socket have to be shared between the polling task and the KcpSocket instance. So it has to be an Arc<>.

I'm sorry, I didn't express it clearly

The original code is : pub struct KcpSocket { kcp: Kcp<UdpOutput>, last_update: Instant, socket: Arc<UdpSocket>, flush_write: bool, flush_ack_input: bool, sent_first: bool, pending_sender: Option<Waker>, pending_receiver: Option<Waker>, closed: bool, }

I think it should be : pub struct KcpSocket<S: AsyncRead + AsyncWrite + Unpin> { kcp: Kcp<UdpOutput>, last_update: Instant, socket: Arc<S>, flush_write: bool, flush_ack_input: bool, sent_first: bool, pending_sender: Option<Waker>, pending_receiver: Option<Waker>, closed: bool, }

There are only two references to socket: socket.send_to() in line 61 socket.try_send_to in line 78

aa51513 commented 1 year ago

The following code is from tokio-tungstenite 0.17.2: pub async fn client_async<'a, R, S>( request: R, stream: S, ) -> Result<(WebSocketStream<S>, Response), WsError> where R: IntoClientRequest + Unpin, S: AsyncRead + AsyncWrite + Unpin, { client_async_with_config(request, stream, None).await }

The stream refers to the tcp connection channel, but it does not mandate that it must be a TcpSocket

I think this should be a good practice

aa51513 commented 1 year ago

The reason I'm filing this issue is that I'm trying to integrate the tokio_kcp library into a tool, but I'm having trouble with the framework

I can't convert an AsyncRead + AsyncWrite + Unpin type to tokio::net::UdpSocket type

After reading the source code, I don't think it is necessary to force the UdpSocket type

As a Rust novice, I would love to hear from you

zonyitoo commented 1 year ago

If I understand clearly, you are going to put an UdpSocket-like type to KcpSocket.

AsyncRead + AsyncWrite is usually implemented for stream oriented sockets, like TcpStream, KcpSocket, but not datagram sockets, like UdpSocket.

We may define a trait like:

trait KcpTransport {
    // Async APIs
    fn poll_send(self: Pin<&mut Self>, buf: &[u8]) -> Poll<io::Result<usize>>;
    fn poll_send_to(self: Pin<&mut Self>, addr: SocketAddr, buf: &[u8]) -> Poll<io::Result<usize>>;
    fn poll_recv(self: Pin<&mut Self>, buf: &mut [u8]) -> Poll<io::Result<usize>>;
    fn poll_recv_from(self: Pin<&mut Self>, buf: &mut [u8]) -> Poll<io::Result<(usize, SocketAddr)>>;

    // Sync APIs
    fn send_to(&mut self, addr: SocketAddr, buf: &[u8]) -> io::Result<usize>;
};
aa51513 commented 1 year ago

If I understand clearly, you are going to put an UdpSocket-like type to KcpSocket.

AsyncRead + AsyncWrite is usually implemented for stream oriented sockets, like TcpStream, KcpSocket, but not datagram sockets, like UdpSocket.

We may define a trait like:

trait KcpTransport {
    // Async APIs
    fn poll_send(self: Pin<&mut Self>, buf: &[u8]) -> Poll<io::Result<usize>>;
    fn poll_send_to(self: Pin<&mut Self>, addr: SocketAddr, buf: &[u8]) -> Poll<io::Result<usize>>;
    fn poll_recv(self: Pin<&mut Self>, buf: &mut [u8]) -> Poll<io::Result<usize>>;
    fn poll_recv_from(self: Pin<&mut Self>, buf: &mut [u8]) -> Poll<io::Result<(usize, SocketAddr)>>;

    // Sync APIs
    fn send_to(&mut self, addr: SocketAddr, buf: &[u8]) -> io::Result<usize>;
};

Thank you ,I got it