SiyaoIsHiding commented on PR #1743: URL: https://github.com/apache/cassandra-java-driver/pull/1743#issuecomment-1796372202
We explored several alternatives for unit testing this memory leak fix but decided to give up testing it, as we could not find a reliable way to test the garbage collector's behavior. ### The unit test we wrote and considered: https://github.com/apache/cassandra-java-driver/blob/cb61eb8c34d1dd7417c03fec95d6e13e9185d537/core/src/test/java/com/datastax/oss/driver/internal/core/metadata/LoadBalancingPolicyWrapperMemoryLeakTest.java#L118-L142 This test: 1. creates two `DefaultNode` 2. creates two `WeakReference` pointing to the nodes, just to poke their existence later 3. initializes the policy 4. clear all the strong references 5. requests for garbage collection 6. verify the node is collected ### We checked that 1. In my local environment (Zulu 8.72.0.17-CA-macos-aarch64), this test will succeed, and if I revert the changes from `WeakHashMap` to the strong `HashMap`, this test will fail. 2. Before all the strong references are cleared, poking the memory, the referring objects to the `weakNode2` are: a. `weakNode2` and `weakReference2` b. `InterceptedInvocation`s by Mockito c. `HashMap` of `allNodes` stored in `when(metadata.getNodes()).thenReturn(allNodes);` d. wanted in `Equals` statement in ` await().atMost(10, TimeUnit.SECONDS).until(() -> weakReference2.get() == null);` These are all expected and no reference is leaked. 3. If `evaluateDistance` of nodes does not return `IGNORED`, they will be stored in `BasicLoadBalancingPolicy.liveNodes`. But nodes there can be removed later by `onDown` or `onRemoved`. We suppose this is intended. ### We considered that According to [this](https://stackoverflow.com/questions/11174328/testing-weakreference/36917692) post, `System.gc()` is more like a request/hint that some JVM will ignore, and there is no reliable way to force garbage collection. This means the test above may fail in other environments, but the last thing we want is a flaky test. We think workarounds like generating a huge amount of garbage to trigger garbage collection may not be worth it, either. Therefore, we concluded that no test may be the best choice for now, and the checks we perform above may be sufficient. -- 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: commits-unsubscr...@cassandra.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
