Bill commented on a change in pull request #6343:
URL: https://github.com/apache/geode/pull/6343#discussion_r617965935
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java
##########
@@ -1617,8 +1612,18 @@ private void readMessages() {
break;
}
- try {
- ByteBuffer buff = getInputBuffer();
+ try (final ByteBufferSharing inputSharing = inputBufferVendor.open()) {
Review comment:
Before this PR, `getInputBuffer()` was called solely by this method.
Once on what was line
[1595](https://github.com/apache/geode/pull/6343/files#diff-69e134fe162fb8f45afd6d09f5cfc91ded364e987dac14c770a963736c431124L1595)
and again on what was line
[1621](https://github.com/apache/geode/pull/6343/files#diff-69e134fe162fb8f45afd6d09f5cfc91ded364e987dac14c770a963736c431124L1621).
The earlier call was moved down to what's now line
[1636](https://github.com/apache/geode/pull/6343/files#diff-69e134fe162fb8f45afd6d09f5cfc91ded364e987dac14c770a963736c431124L1621).
What was the later call is now right after this comment. The code is a direct
copy/paste of the old `getInputBuffer()` code.
So in this way I feel like this was a mere refactoring. Doesn't mean the
resulting code is "good" but it should mean it behaves the same as before.
Now `expand{Read/Write}BufferIfNeeded()` only ever _expands_ a buffer. And
those methods are called in exactly three places in this class:
1. `createIoFilter()` which is called if this is a sender or receiver doing
TLS
2. `compactOrResizeBuffer()` which is called after receiving a non-TLS
message
3. this code in `readMessages()` which can be called to receive a message
in both the TLS and non-TLS case
So I think I just convinced myself that this code below that neither of us
likes, is only executed in the non-TLS case. And as a result, I think your
concern about this allocation running afoul of buffer sizes in the TLS case, is
addressed.
I'd like this a lot better if the size calculation could be moved into the
(2) constructors and we could pre-allocate the right size buffer. I see that
`recvBufferSize` is set (indirectly) during construction so that one's
available. And the `owner` is certainly available.
How would you feel about me taking a whack at moving this into the
constructors? I feel like I can move the TLS buffer size calculation into the
constructors too. If I can do both of those we can eliminate the whole
null-handling complication (in byte buffer sharing) entirely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
