EsotericSoftware / EsotericSoftware/kryonet
Feature Request: Avoid write buffer overflow by blocking on TcpConnection.send and drain writeBuffer when it approaches high-water mark
- Dominant language
- Java
- Stars
- 1.9k
- Forks
- 411
- PR merge metrics
- No merged PRs in 30d
Description
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.
``` java
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.
``` java
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:
``` java
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.
Contributor guide
Assessment
This issue has not been assessed yet.