Skip to content
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

[fix][tests] Fix Mockito mocks memory leak #17851

Merged
merged 10 commits into from
Sep 27, 2022

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Sep 27, 2022

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

  • doc-required
    (Your PR needs to update docs and you will update later)

  • 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: lhotari#92

@lhotari lhotari force-pushed the lh-attempt-to-fix-mockito-memory-leak branch from dd5e115 to 9b70123 Compare September 27, 2022 09:06
@lhotari lhotari marked this pull request as draft September 27, 2022 09:41
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 27, 2022
@lhotari lhotari marked this pull request as ready for review September 27, 2022 12:20
@lhotari lhotari requested review from tisonkun and nicoloboschi and removed request for tisonkun September 27, 2022 12:20
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Since we already hard depend on org.mockito.internal.util.MockUtil, the comment above is not a blocker.

@tisonkun
Copy link
Member

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 :)

@lhotari
Copy link
Member Author

lhotari commented Sep 27, 2022

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.

@tisonkun
Copy link
Member

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.

@lhotari
Copy link
Member Author

lhotari commented Sep 27, 2022

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 verification or stubbing 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.

org.apache.pulsar.tests.MockitoThreadLocalStateCleaner will log warnings about "Invalid usage of Mockito detected on thread ..." to help find the locations of the misuse of Mockito in Pulsar tests.
Similarly it's now logging "Mock contains registered invocations that should be cleared". This is something that should be fixed in tests so that it doesn't leak.

I guess it would be helpful to have some support for checks like this directly in Mockito so that it would be possible to add such validation without using reflection.

@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.

@lhotari lhotari merged commit d4893d1 into apache:master Sep 27, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Dec 7, 2022
* Call cleanup method in finally block to ensure it's not skipped

* Clear invocations for the mocks that are left around without cleanup

* Cleanup PulsarService and PulsarAdmin mocks/spies in MockedPulsarServiceBaseTest

* Don't record invocations at all for PulsarService and PulsarAdmin in MockedPulsarServiceBaseTest

* Don't record invocations for spies by default

* Simplify reseting mocks

* Fix PersistentTopicTest

* Fix TokenExpirationProducerConsumerTest

* Fix SimpleLoadManagerImplTest

* Fix FilterEntryTest

(cherry picked from commit d4893d1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs type/flaky-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: Out-of-memory error in Broker Group 3 unit tests
3 participants