-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure resizing of the sendBuffer #746
Conversation
dataPort.write(sendBuffer, sendPosition); | ||
connection.getNatsStatistics().registerWrite(sendPosition); | ||
sendPosition = 0; | ||
msg = msg.next; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's not obvious, the NatMessage has a field called next, which is really a linked list. You have to get the next message or you would just infinite loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah exactly. But the thing is that this msg = msg.next;
statement is already done at the end of the loop. (The msg = msg.next
that is highlighted only gets called when the sendBuffer
doesn't have enough space)
I believe this line removes the current message, doesn't send it, and just skips to the next message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also my comment here, which is a more detailed explanation:
#644 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also commented in 644. I get it now. This is another option, at most the loop will execute twice. Both options are probably equivla
while (sendPosition + size > sendBuffer.length) {
if (sendPosition == 0) { // have to resize
this.sendBuffer = new byte[(int)Math.max(sendBuffer.length + size, sendBuffer.length * 2L)];
} else { // else send and go to next message
dataPort.write(sendBuffer, sendPosition);
connection.getNatsStatistics().registerWrite(sendPosition);
sendPosition = 0;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to ask one other person to check this, make sure I can explain it to them and see if they see it too, but I'm inclined to accept this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A unit test to check this exact condition would be great. Not sure how easy it would be, I think a small options.getBufferSize()
with a message with a payload larger (double to be safe?) might do the trick.
int bufSize = options.getBufferSize();
this.sendBuffer = new byte[bufSize];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, not quite sure how to test this. As this case would only be triggered if at least one message was sent prior in this batch (so the sendPosition > 0
). So you'd need one small(er) message that would fit in the buffer to move the sendPosition
, and then another bigger message that wouldn't fit in the buffer.
I believe there was an error in the calculation of the Added a commit, including a test which adds coverage for calculating non-protocol message sizes. 640adf6 |
} | ||
if (dataLen > 0) { | ||
sizeInBytes += dataLen; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this change. We can look a this another time.
I believe there is an issue related to resizing the
sendBuffer
during thesendMessageBatch
inNatsConnectionWriter
.This should fix an
ArrayIndexOutOfBoundsException
that could occur when thesendBuffer
is (not) resized during specific scenarios.A more detailed explanation of the issue:
#644 (comment)