Fix long message handling with dynamic buffer growth #158
Conversation
The socket transport plugin used a fixed 64KB buffer size which caused messages surpassing that size to be truncated For UDP/Unix datagram sockets, this resulted in parsing errors like "unexpected end of input" This change allows the buffer to grow (up to a limit depending on the protocol) to accommodate larger messages. Closes: OSPRH-23826
|
Still working on unit tests |
Use sendTCPSocketMessage name instead
vyzigold
left a comment
vyzigold
left a comment
There was a problem hiding this comment.
I see one thing in the tests (see my comment) which needs a change / discussion. But otherwise this looks quite good. Thank you for the cleanup of the tests!
| copy(data[len(remainingMsg):], msgBuffer[:n]) | ||
| } else { | ||
| data = msgBuffer[:n] | ||
| } |
There was a problem hiding this comment.
This seems to me like the same as the "append" line that was here previously. But I like this, copy might behave better to the memory then appending a large string (not sure how exactly append is imlemented in go unfortunately).
There was a problem hiding this comment.
For what I could understood from https://go.dev/ref/spec#Appending_and_copying_slices, "append(remainingMsg, msgBuffer...)" will append the remaining message, whatever remainder that is on the buffer (which may be 1 byte) and then just fill to complete the maxBufferSize with zeroes. So in every iteration we allocate extra memory without the need of doing so.
Using copy we only allocate what we need and it is a bit easier on resources.
| assert.Equal(t, true, len(receivedMsg) > 0) | ||
|
|
||
| // If we received a complete message, verify the content | ||
| if len(receivedMsg) == largeBuffSize+len(addition) { |
There was a problem hiding this comment.
Hmm. Does it actually get inside this if? If I understand it correctly. You're sending a 128kb message, but the socket buffer hasn't expanded yet (it's still set to 64kb), so a truncated string is received and sent further (which means only the first half of the sent message gets assigned to receivedMsg right?). So this condition would be 64kb == 128kb + 19. So even though the test passes, I don't think the code inside the condition ever executes.
What I think should happen in this testcase:
- Send the large message +
addition - Optionally check that receivedMsg only has the first half of the message
- Send the same message again
- Check that we now have 128kb of data with the last character being '$'
- Send the same message again
- Check that we now have 128kb + 19 bytes of data and the end is "wubba lubba dub dub"
There was a problem hiding this comment.
Good catch! Indeed, we should extend this test to send a large message, catch the log message and verify we only received the base buffer size and then retry sending a long message and see it succeeds. I will fix this.
| sendTCPSocketMessage(t, logger, "127.0.0.1:8661", 100000, 'B', []byte("--LARGE-TCP--")) | ||
| }) | ||
|
|
||
| t.Run("test multiple large messages", func(t *testing.T) { |
There was a problem hiding this comment.
I think having multiple messages right after each other and so reading a beginning of the next message together with the end of the last message was an issue at some point. Good idea to add this test 👍
|
Thanks for the review! |
This test verifies the dynamic buffer growth by sending three messages In each iteration the buffer grows from the initial size of 65535 bytes to 3 times the initial size. Also verifies the content of the received message
| msgLengthSize = 8 | ||
| maxBufferSize = 65535 // 64KB - initial buffer size for all socket types and max for UDP (OS datagram limit) | ||
| maxBufferSizeUnix = 10485760 // 10MB - max buffer size for Unix domain sockets | ||
| maxBufferSizeTCP = 104857600 // 100MB - max buffer size for TCP (stream-based, can handle very large messages) |
There was a problem hiding this comment.
Not sure about this limit, maybe too high?
| ) | ||
|
|
||
| const regularBuffSize = 16384 | ||
| const regularBuffSize = 65535 // default buffer size |
There was a problem hiding this comment.
Adjusted with the buffer size we are using, maybe this is not needed
| copy(data[len(remainingMsg):], msgBuffer[:n]) | ||
| } else { | ||
| data = msgBuffer[:n] | ||
| } |
There was a problem hiding this comment.
For what I could understood from https://go.dev/ref/spec#Appending_and_copying_slices, "append(remainingMsg, msgBuffer...)" will append the remaining message, whatever remainder that is on the buffer (which may be 1 byte) and then just fill to complete the maxBufferSize with zeroes. So in every iteration we allocate extra memory without the need of doing so.
Using copy we only allocate what we need and it is a bit easier on resources.
| assert.Equal(t, true, len(receivedMsg) > 0) | ||
|
|
||
| // If we received a complete message, verify the content | ||
| if len(receivedMsg) == largeBuffSize+len(addition) { |
There was a problem hiding this comment.
Good catch! Indeed, we should extend this test to send a large message, catch the log message and verify we only received the base buffer size and then retry sending a long message and see it succeeds. I will fix this.
The socket transport plugin used a fixed 64KB buffer size which caused messages surpassing that size to be truncated
For UDP/Unix datagram sockets, this resulted in parsing errors like "unexpected end of input"
This change allows the buffer to grow (up to a limit depending on the protocol) to accommodate larger messages.
Closes: OSPRH-23826