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.

Sign up for GitHub

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

Jump to bottom

Revert "[fix][ci] Fix tests memory leak due to mockito-inline (#15513)" #16820

Closed
wants to merge 1 commit into from
Closed

Revert "[fix][ci] Fix tests memory leak due to mockito-inline (#15513)" #16820

wants to merge 1 commit into from

Conversation

nodece
Copy link
Member

@nodece nodece commented Jul 27, 2022

This reverts commit 14b95ec.

Fixes #16582

Motivation

#15513 uses clearInlineMocks() method to fix the memory leak, but it looks to introduce a new issue:

org.mockito.exceptions.base.MockitoException: Unable to create mock instance of type 'ServerCnx'
  	at org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgs(BrokerTestUtil.java:43)
  	at org.apache.pulsar.broker.service.PersistentTopicTest.setup(PersistentTopicTest.java:226)
  	at org.apache.pulsar.broker.service.persistent.PersistentTopicStreamingDispatcherTest.setup(PersistentTopicStreamingDispatcherTest.java:34)
  	at jdk.internal.reflect.GeneratedMethodAccessor599.invoke(Unknown Source)
  	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
  	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
  	at org.testng.internal.MethodInvocationHelper.invokeMethodConsideringTimeout(MethodInvocationHelper.java:61)
  	at org.testng.internal.ConfigInvoker.invokeConfigurationMethod(ConfigInvoker.java:366)
  	at org.testng.internal.ConfigInvoker.invokeConfigurations(ConfigInvoker.java:320)
  	at org.testng.internal.TestInvoker.runConfigMethods(TestInvoker.java:701)
  	at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:527)
  	at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
  	at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
  	at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
  	at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
  	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
  	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
  	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
  	at org.testng.TestRunner.privateRun(TestRunner.java:764)
  	at org.testng.TestRunner.run(TestRunner.java:585)
  	at org.testng.SuiteRunner.runTest(SuiteRunner.java:384)
  	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378)
  	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337)
  	at org.testng.SuiteRunner.run(SuiteRunner.java:286)
  	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
  	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
  	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218)
  	at org.testng.TestNG.runSuitesLocally(TestNG.java:1140)
  	at org.testng.TestNG.runSuites(TestNG.java:1069)
  	at org.testng.TestNG.run(TestNG.java:1037)
  	at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
  	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeSingleClass(TestNGDirectoryTestSuite.java:112)
  	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeLazy(TestNGDirectoryTestSuite.java:123)
  	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:90)
  	at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
  	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
  	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
  	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
  	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
  Caused by: org.mockito.creation.instance.InstantiationException: 
  Unable to create instance of 'ServerCnx'.
  Please ensure the target class has a constructor that matches these argument types: [org.apache.pulsar.broker.PulsarService] and executes cleanly.
  	... 40 more
  Caused by: java.lang.reflect.InvocationTargetException
  	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:198)
  	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:161)
  	at org.mockito.internal.util.reflection.ModuleMemberAccessor.newInstance(ModuleMemberAccessor.java:42)
  	at org.mockito.internal.creation.instance.ConstructorInstantiator.invokeConstructor(ConstructorInstantiator.java:70)
  	at org.mockito.internal.creation.instance.ConstructorInstantiator.withParams(ConstructorInstantiator.java:53)
  	at org.mockito.internal.creation.instance.ConstructorInstantiator.newInstance(ConstructorInstantiator.java:39)
  	at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.doCreateMock(InlineDelegateByteBuddyMockMaker.java:360)
  	at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.createMock(InlineDelegateByteBuddyMockMaker.java:330)
  	at org.mockito.internal.creation.bytebuddy.InlineByteBuddyMockMaker.createMock(InlineByteBuddyMockMaker.java:58)
  	at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:53)
  	at org.mockito.internal.MockitoCore.mock(MockitoCore.java:84)
  	at org.mockito.Mockito.mock(Mockito.java:1964)
  	... 40 more
  Caused by: java.lang.ClassCastException: class org.apache.pulsar.broker.service.BrokerService cannot be cast to class org.apache.pulsar.broker.resources.PulsarResources (org.apache.pulsar.broker.service.BrokerService and org.apache.pulsar.broker.resources.PulsarResources are in unnamed module of loader 'app')
  	at org.apache.pulsar.broker.PulsarService.getPulsarResources(PulsarService.java:265)
  	at org.apache.pulsar.broker.service.TopicListService.<init>(TopicListService.java:103)
  	at org.apache.pulsar.broker.service.ServerCnx.<init>(ServerCnx.java:279)
  	at org.apache.pulsar.broker.service.ServerCnx.<init>(ServerCnx.java:239)
  	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
  	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor$Dispatcher$ByteBuddy$kV10z9EX.invokeWithArguments(Unknown Source)
  	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.lambda$newInstance$0(InstrumentationMemberAccessor.java:191)
  	at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:188)
  	... 51 more

I see a description of clearInlineMocks() that seems to be relevant to this problem, so try to revert #15513.

In certain specific, rare scenarios (issue #1619) inline mocking causes memory leaks. There is no clean way to mitigate this problem completely. Hence, we introduced a new API to explicitly clear mock state (only make sense in inline mocking!). See example usage in MockitoFramework.clearInlineMocks(). If you have feedback or a better idea how to solve the problem please reach out.

Documentation

Check the box below or label this PR directly.

Need to update docs?

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

@nodece nodece requested review from nicoloboschi and removed request for nicoloboschi July 27, 2022 11:01
nicoloboschi
nicoloboschi reviewed Jul 27, 2022
View reviewed changes
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

I don't see any suggestion in the javadoc that indicates the usage of clearInlineMocks() could cause the error we're facing, would you mind to indicate more details? @nodece

@nodece
Copy link
Member Author

nodece commented Jul 27, 2022

I don't see any suggestion in the javadoc that indicates the usage of clearInlineMocks() could cause the error we're facing, would you mind to indicate more details? @nodece

I just guess the log is related to #15513.

In certain specific, rare scenarios (issue mockito/mockito#1619) inline mocking causes memory leaks. There is no clean way to mitigate this problem completely. Hence, we introduced a new API to explicitly clear mock state (only make sense in inline mocking!). See example usage in MockitoFramework.clearInlineMocks(). If you have feedback or a better idea how to solve the problem please reach out.

Link to https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#47

@shibd
Copy link
Member

shibd commented Jul 27, 2022

Hi, @nodece. About ClassCastException , in #16821 will fixed. I tested the code after the revert and an exception still will appear.

@nodece
Copy link
Member Author

nodece commented Jul 28, 2022

@shibd Thank you for your testing, I close this PR.

@nodece nodece closed this Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicoloboschi nicoloboschi nicoloboschi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test: mockito broken
3 participants
@nodece @shibd @nicoloboschi