lemunozm / message-io

Fast and easy-to-use event-driven network library.
Apache License 2.0
1.11k stars 74 forks source link

TCP, FramedTCP message send is not truly async #177

Open PPakalns opened 1 month ago

PPakalns commented 1 month ago

As seen in TODO comment and behaviour on io::ErrorKind::WouldBlock.

In server case this blocks communication with other clients.

I stumbled upon a situation when a faulty virtual network tunnel doesn't process sent data and whole program gets stuck in this loop and has 100% CPU utilization. It causes issues and delays in other situations too with large messages.

https://github.com/lemunozm/message-io/blob/master/src/adapters/tcp.rs#L187


    fn send(&self, data: &[u8]) -> SendStatus {
        // TODO: The current implementation implies an active waiting,
        // improve it using POLLIN instead to avoid active waiting.
        // Note: Despite the fear that an active waiting could generate,
        // this only occurs in the case when the receiver is full because reads slower that it sends.
        let mut total_bytes_sent = 0;
        loop {
            let mut stream = &self.stream;
            match stream.write(&data[total_bytes_sent..]) {
                Ok(bytes_sent) => {
                    total_bytes_sent += bytes_sent;
                    if total_bytes_sent == data.len() {
                        break SendStatus::Sent
                    }
                }
                Err(ref err) if err.kind() == io::ErrorKind::WouldBlock => continue,

                // Others errors are considered fatal for the connection.
                // a Event::Disconnection will be generated later.
                Err(err) => {
                    log::error!("TCP receive error: {}", err);
                    break SendStatus::ResourceNotFound // should not happen
                }
            }
        }
    }
lemunozm commented 1 month ago

Hi, thanks for bringing this issue to light!

Yes, if the way of unlocking the receiver queue is in the same thread as this sender, then it will cause 100% of the CPU without the possibility of unlocking it. That should definitely be fixed using a POLLIN

PPakalns commented 1 month ago

Workaround implementation of AsyncFrameAdapter: https://github.com/lemunozm/message-io/compare/master...PPakalns:message-io:async_framed_tcp

Of course, this adds additional cost of storing queue of buffers to be sent over the network.

lemunozm commented 1 month ago

Great! Feel free to open a PR with this new adapter. It can be interesting for users using tcp as you.