You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/09/27 09:02:46 UTC
[GitHub] [pulsar] lhotari opened a new pull request, #17851: [fix][tests] Fix Mockito mocks memory leak
lhotari opened a new pull request, #17851:
URL: https://github.com/apache/pulsar/pull/17851
Fixes #17714
### Motivation
There's a memory leak when using Mockito mocks since all invocations get recorded.
### Modifications
Add multiple solutions for making sure that mocks are cleaned up properly.
Modify MockedPulsarServiceBaseTest to clean up PulsarService and PulsarAdmin mocks since the detected memory leaks are caused by these mocks.
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#discussion_r980987160
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java:
##########
@@ -44,4 +44,19 @@ public static <T> T spyWithClassAndConstructorArgs(Class<T> classToSpy, Object..
.useConstructor(args)
.defaultAnswer(Mockito.CALLS_REAL_METHODS));
}
+
+ /**
+ * Create a Mockito spy that is stub-only which does not record method invocations,
+ * thus saving memory but disallowing verification of invocations.
Review Comment:
> but disallowing verification of invocations
If so, can we replace those calls to `spy(obj)` with bare `obj`?
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#issuecomment-1259763494
> > There's nothing here that is caused by a Mockito bug.
>
> I have some experience in asking upstream experts for best practices. But let's say I should volunteer to report the case instead of requesting others.
In this case, the reason for the memory leaks is the misuse of Mockito in the Pulsar internal test framework.
Mockito will record invocations by default. To clear the invocations, mocks should be resetted or invocations should be cleared if the instances are around so that it matters.
[Unfinished stubbing](https://github.com/mockito/mockito/wiki/FAQ#what-are-unfinished-verificationstubbing-errors) will also leak memory. Those are really issues in test code of Pulsar, but nobody has cared to fix the warning that are logged by the test interceptors that mitigate the bad practices by clearing the thread locals.
@tisonkun Since you seem to care about this topic, it would be useful to fix issues in Pulsar test code one issue at a time.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#discussion_r981205864
##########
buildtools/src/main/java/org/apache/pulsar/tests/MockitoThreadLocalStateCleaner.java:
##########
@@ -72,16 +74,33 @@ public void cleanup() {
LOG.warn("Invalid usage of Mockito detected on thread {}."
+ " There is ongoing stubbing on mock of class={} instance={}",
thread, mock.getClass().getName(), mock);
+ try {
+ clearInvocations(thread, mock);
+ } catch (Exception e) {
+ LOG.warn("Clearing invocations failed", e);
+ }
Review Comment:
What's the difference between this private method and:
```java
import static org.mockito.Mockito.clearInvocations;
try {
clearInvocations(thread, mock);
} catch (Exception e) {
LOG.warn("Clearing invocations failed", e);
}
```
I may tend to avoid importing something from `internal` package.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#discussion_r981224614
##########
buildtools/src/main/java/org/apache/pulsar/tests/MockitoThreadLocalStateCleaner.java:
##########
@@ -72,16 +74,33 @@ public void cleanup() {
LOG.warn("Invalid usage of Mockito detected on thread {}."
+ " There is ongoing stubbing on mock of class={} instance={}",
thread, mock.getClass().getName(), mock);
+ try {
+ clearInvocations(thread, mock);
+ } catch (Exception e) {
+ LOG.warn("Clearing invocations failed", e);
+ }
Review Comment:
This is not something that is used in developer written tests. The solution will log warnings when tests aren't doing the right thing. Tests should call `Mockito.reset` for mocks.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
lhotari merged PR #17851:
URL: https://github.com/apache/pulsar/pull/17851
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#discussion_r980987160
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java:
##########
@@ -44,4 +44,19 @@ public static <T> T spyWithClassAndConstructorArgs(Class<T> classToSpy, Object..
.useConstructor(args)
.defaultAnswer(Mockito.CALLS_REAL_METHODS));
}
+
+ /**
+ * Create a Mockito spy that is stub-only which does not record method invocations,
+ * thus saving memory but disallowing verification of invocations.
Review Comment:
> but disallowing verification of invocations
If so, can we replace `spy(obj)` with bare `obj`?
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#discussion_r981016007
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java:
##########
@@ -44,4 +44,19 @@ public static <T> T spyWithClassAndConstructorArgs(Class<T> classToSpy, Object..
.useConstructor(args)
.defaultAnswer(Mockito.CALLS_REAL_METHODS));
}
+
+ /**
+ * Create a Mockito spy that is stub-only which does not record method invocations,
+ * thus saving memory but disallowing verification of invocations.
Review Comment:
no. stubbing is still used in the current solution.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#issuecomment-1259510244
> Perhaps we can report this OOM to the mockito community for a solution in idiom. Since they mark these methods as not recommended they could have some insights here :)
There's nothing here that is caused by a Mockito bug.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#discussion_r981044595
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/BrokerTestUtil.java:
##########
@@ -44,4 +44,19 @@ public static <T> T spyWithClassAndConstructorArgs(Class<T> classToSpy, Object..
.useConstructor(args)
.defaultAnswer(Mockito.CALLS_REAL_METHODS));
}
+
+ /**
+ * Create a Mockito spy that is stub-only which does not record method invocations,
+ * thus saving memory but disallowing verification of invocations.
Review Comment:
Stubbing is used to mock over real methods?
~~At least for `admin = spy(...)` I don't find a call to mock over real methods.~~ Well. One reference at `org.apache.pulsar.broker.admin.NamespacesTest#testDeleteNamespaceWithBundles` L816.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#issuecomment-1259479257
Perhaps we can report this OOM to the mockito community for a solution in idiom. Since they mark these methods as not recommended they could have some insights here :)
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#discussion_r981015537
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java:
##########
@@ -304,11 +307,19 @@ protected void stopBroker() throws Exception {
// set shutdown timeout to 0 for forceful shutdown
pulsar.getConfiguration().setBrokerShutdownTimeoutMs(0L);
pulsar.close();
+ cleanupMock(pulsar);
pulsar = null;
// Simulate cleanup of ephemeral nodes
//mockZooKeeper.delete("/loadbalance/brokers/localhost:" + pulsar.getConfiguration().getWebServicePort(), -1);
}
+ protected void cleanupMock(Object mock) {
Review Comment:
I'm simplifying this. I'll push an update soon. It's sufficient to call Mockito.reset
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#issuecomment-1259542079
> There's nothing here that is caused by a Mockito bug.
I have some experience in asking upstream experts for best practices. But let's say I should volunteer to report the case instead of requesting others.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nicoloboschi commented on a diff in pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
nicoloboschi commented on code in PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#discussion_r980978416
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java:
##########
@@ -304,11 +307,19 @@ protected void stopBroker() throws Exception {
// set shutdown timeout to 0 for forceful shutdown
pulsar.getConfiguration().setBrokerShutdownTimeoutMs(0L);
pulsar.close();
+ cleanupMock(pulsar);
pulsar = null;
// Simulate cleanup of ephemeral nodes
//mockZooKeeper.delete("/loadbalance/brokers/localhost:" + pulsar.getConfiguration().getWebServicePort(), -1);
}
+ protected void cleanupMock(Object mock) {
Review Comment:
what about adding this method to `BrokerTestUtil` ?
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a diff in pull request #17851: [fix][tests] Fix Mockito mocks memory leak
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17851:
URL: https://github.com/apache/pulsar/pull/17851#discussion_r981019222
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockedPulsarServiceBaseTest.java:
##########
@@ -304,11 +307,19 @@ protected void stopBroker() throws Exception {
// set shutdown timeout to 0 for forceful shutdown
pulsar.getConfiguration().setBrokerShutdownTimeoutMs(0L);
pulsar.close();
+ cleanupMock(pulsar);
pulsar = null;
// Simulate cleanup of ephemeral nodes
//mockZooKeeper.delete("/loadbalance/brokers/localhost:" + pulsar.getConfiguration().getWebServicePort(), -1);
}
+ protected void cleanupMock(Object mock) {
Review Comment:
Changes pushed
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org