divijvaidya commented on code in PR #12228:
URL: https://github.com/apache/kafka/pull/12228#discussion_r902801885
##########
clients/src/main/java/org/apache/kafka/common/network/Selector.java:
##########
@@ -809,6 +810,13 @@ private void maybeCloseOldestConnection(long
currentTimeNanos) {
* poll().
*/
public void clearCompletedReceives() {
+ this.completedReceives.values().forEach(networkReceive -> {
+ try {
+ networkReceive.close();
Review Comment:
ok, so I was able to write a test that existing code fails by adding a
method in `GarbageCollectedMemoryPool` which validates that all buffer has been
released. But the memory pool in a selector is not used by anything other than
tests. So, production won't have a memory leak here.
The ownership of lifecycle of `NetworkReceive` is confusing and not well
defined in the current code. Currently, the list of `CompletedNetworkReceive`
is cleared on every poll event (without closing the `NetworkReceive`). Before
the poll, a potential caller can obtain reference to `NetworkReceive` using the
`Selector.completedReceives()` method. If we close the `NetworkReceive`, the
reference available with the caller will not be valid. Which begs the question,
is the Selector responsible for closing completed `NetworkReceive(s)` or is it
the caller of `Selector.completedReceives()`. The test
`SelectorTest.testMuteOnOOM` relies on the fact that caller is responsible for
closing the `NetworkReceive`. Although, personally, that doesn't look right to
me but nevertheless I don't want to have a discussion about that in the scope
of this PR. Since MemoryPool backed `NetworkReceive` is not used by any
non-test classes, I am reverting my changes to this class from this PR and will
open a
separate JIRA ticket to have the discussion about lifecycle ownership.
I hope the above makes sense?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
