Feature Request: Avoid write buffer overflow by blocking on TcpConnection.send and drain writeBuffer when it approaches high-water mark

I have a server using Kryonet RMI that is hitting a transient buffer overflow from TcpConnection.send. This occurs when the server attempts to send data faster than it can be written to the socket. Currently an exception is thrown and the connection closes or the server crashes.

My current writeBuffer is 1MB. This server needs to handle many connections so the writeBuffer can't grow too large. Closing the socket in this case is not an option.

It would be useful if there was an option to block on send only when the writeBuffer is approaching a high water mark and drain the writeBuffer until it hits a low water mark. I have implemented this feature on a local copy of the code. The following code changes in TcpConnection worked for me but I'm sure there is a better way to implement this.

TcpConnection.writeToSocket takes in a parameter to specify whether to block on write or not. If blocking on write is enabled then it will drain the buffer to the socket until the low water mark is hit.

    private boolean writeToSocket (final boolean blocking) throws IOException {
        SocketChannel socketChannel = this.socketChannel;
        if (socketChannel == null) throw new SocketException("Connection is closed.");

        ByteBuffer buffer = writeBuffer;
        buffer.flip();
        while (buffer.hasRemaining()) {
            if (bufferPositionFix) {
                buffer.compact();
                buffer.flip();
            }

            // if no bytes were written to the socket (the socket buffer is full) ...
            if (socketChannel.write(buffer) == 0) {
                // ... and if we should block and we have not reached the low water mark 
                if (blocking && (buffer.position() / (float) buffer.limit()) > WRITE_BUFFER_LOW) {    
                    // ... then yield the thread and continue attempting to drain the buffer
                    Thread.yield();

                    // NOTE: Thread.sleep() might be a more appropriate option, but how long
                    // should the thread sleep for?

                } else {
                    // ... otherwise don't block and return
                    break;
                }
            }
        }

        buffer.compact();

        return buffer.position() == 0;
    }

TcpConnection.send checks writeBuffer to see if it's above the high water mark and calls the blocking version of writeToSocket to drain the buffer.

    public int send (Connection connection, Object object) throws IOException {
        SocketChannel socketChannel = this.socketChannel;
        if (socketChannel == null) throw new SocketException("Connection is closed.");
        synchronized (writeLock) {
            // check to see if buffer is approaching capacity to help avoid buffer overflow
            while ((writeBuffer.position() / (float)writeBuffer.limit()) > WRITE_BUFFER_HIGH) {
                writeToSocket(true);
            }

            // Leave room for length.
            int start = writeBuffer.position();
            int lengthLength = serialization.getLengthLength();
            writeBuffer.position(writeBuffer.position() + lengthLength);

            // REST OF METHOD OMITTED FOR CLARITY
        }
    }

I'm using the following values for the high and low water marks expressed in a fraction of the buffer used:

    private static final double WRITE_BUFFER_HIGH = 0.75;
    private static final double WRITE_BUFFER_LOW = 0.25;

The write buffer size and high water mark are selected such that the largest object expected to be sent would fit within the remaining buffer. In my case the high water mark is 75% and the buffer is 1MB. Therefore the largest object I can safely send without worry of a buffer overflow is 0.25MB (the remaining space in the write buffer when we are nearly at the high water mark).

I currently have the low water mark at 25%, but now that I think of it the low water mark could be the same as the high water mark as its not really necessary to keep blocking and draining the writeBuffer beyond the high water mark.

It seems like a reasonable feature, disabled by default of course. I agree, probably don't need a low water mark. If the high water mark were in bytes, we wouldn't need a division (FWIW).

Can you submit a PR?

On Thu, Sep 11, 2014 at 7:29 AM, Luke Valenty notifications@github.com wrote:

I have a server using Kryonet RMI that is hitting a transient buffer overflow from TcpConnection.send. This occurs when the server attempts to send data faster than it can be written to the socket. Currently an exception is thrown and the connection closes or the server crashes.

My current writeBuffer is 1MB. This server needs to handle many connections so the writeBuffer can't grow too large. Closing the socket in this case is not an option.

It would be useful if there was an option to block on send only when the writeBuffer is approaching a high water mark and drain the writeBuffer until it hits a low water mark. I have implemented this feature on a local copy of the code. The following code changes in TcpConnection worked for me but I'm sure there is a better way to implement this.

TcpConnection.writeToSocket takes in a parameter to specify whether to block on write or not. If blocking on write is enabled then it will drain the buffer to the socket until the low water mark is hit.
private boolean writeToSocket (final boolean blocking) throws IOException {
    SocketChannel socketChannel = this.socketChannel;
    if (socketChannel == null) throw new SocketException("Connection is closed.");

    ByteBuffer buffer = writeBuffer;
    buffer.flip();
    while (buffer.hasRemaining()) {
        if (bufferPositionFix) {
            buffer.compact();
            buffer.flip();
        }

        // if no bytes were written to the socket (the socket buffer is full) ...
        if (socketChannel.write(buffer) == 0) {
            // ... and if we should block and we have not reached the low water mark
            if (blocking && (buffer.position() / (float) buffer.limit()) > WRITE_BUFFER_LOW) {
                // ... then yield the thread and continue attempting to drain the buffer
                Thread.yield();

                // NOTE: Thread.sleep() might be a more appropriate option, but how long
                // should the thread sleep for?

            } else {
                // ... otherwise don't block and return
                break;
            }
        }
    }

    buffer.compact();

    return buffer.position() == 0;
}
TcpConnection.send checks writeBuffer to see if it's above the high water mark and calls the blocking version of writeToSocket to drain the buffer.
public int send (Connection connection, Object object) throws IOException {
    SocketChannel socketChannel = this.socketChannel;
    if (socketChannel == null) throw new SocketException("Connection is closed.");
    synchronized (writeLock) {
        // check to see if buffer is approaching capacity to help avoid buffer overflow
        while ((writeBuffer.position() / (float)writeBuffer.limit()) > WRITE_BUFFER_HIGH) {
            writeToSocket(true);
        }

        // Leave room for length.
        int start = writeBuffer.position();
        int lengthLength = serialization.getLengthLength();
        writeBuffer.position(writeBuffer.position() + lengthLength);

        // REST OF METHOD OMITTED FOR CLARITY
    }
}
I'm using the following values for the high and low water marks expressed in a fraction of the buffer used:
private static final double WRITE_BUFFER_HIGH = 0.75;
private static final double WRITE_BUFFER_LOW = 0.25;
The write buffer size and high water mark are selected such that the largest object expected to be sent would fit within the remaining buffer. In my case the high water mark is 75% and the buffer is 1MB. Therefore the largest object I can safely send without worry of a buffer overflow is 0.25MB (the remaining space in the write buffer when we are nearly at the high water mark).

I currently have the low water mark at 25%, but now that I think of it the low water mark could be the same as the high water mark as its not really necessary to keep blocking and draining the writeBuffer beyond the high water mark.

— Reply to this email directly or view it on GitHub https://github.com/EsotericSoftware/kryonet/issues/75.

Working on a PR now. I have a couple other questions:

TcpConnection and UdpConnection don't use direct byte buffers, any reason why? Using direct byte buffers may help reduce IO overhead when writing to/reading from sockets.
UdpConnection will throw KryoNetException when the writeBuffer overflows. Because UDP packets are not guaranteed to reach their destination by the network it would be safe to silently drop the UDP packet. Any application using UDP needs to be able to handle dropped packets on both send and receive anyways. This could be preferable to throwing a KryoNetException or blocking.

Any progress on this at all? I'm willing to help but need to know the current progress :)

EsotericSoftware / kryonet

Feature Request: Avoid write buffer overflow by blocking on TcpConnection.send and drain writeBuffer when it approaches high-water mark #75