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

Memory leak in mockito-inline calling method on mock with at least a mock as parameter #1614

Closed
ttanxu opened this issue Feb 9, 2019 · 9 comments

Comments

@ttanxu
Copy link
Contributor

@ttanxu ttanxu commented Feb 9, 2019

Summary

We found a memory leak with mockito-inline. A short example is shown as below. A more detailed example can be found at the end.

class BigClass {
  void accept(SmallClass small) {}
}

class SmallClass {
  void accept(BigClass big) {}
}

void leak() {
  BigClass a = Mockito.mock(BigClass.class);
  SmallClass b = Mockito.mock(SmallClass.class);

  // Remove any one of the 2 lines below will stop leaking
  a.accept(b);
  b.accept(a);
}

Analysis

Looking at the memory dump. Mock of BigClass and SmallClass are held as a weak ref in a map of type WeakConcurrentMap$WithInlinedExpunction used to map the mock and its invocation handler. When the mock can be reclaimed by GC the map will remove the record from it.

In the case with memory leak, the mock instance of BigClass was held as a strong reference by rawArguments and arguments in InvocationMatcher, as invocation in InterceptedInvocation, as invocationForStubbing in InvocationContainerImpl, as invocationContainer in MockHandlerImpl. That eventually leads to a value in the WeakConcurrentMap for SmallClass. Similar thing happens to the mock instance of SmallClass. That creates a ring of reference and no mock can be reclaimed by GC because they are all referenced transitively by a value in the map.

Similar things can also happen for stubbed methods, saved in stubbed in InvocationContainerImpl.

Subclass mock makers don't suffer from it because there is no map from mock to handler -- it's just a strong reference. GC can handle non-accessible rings well, but GC doesn't know the mock map purging semantic in inline mock makers.

Potential Solution

Unlike #1533 where converting spiedInstance to a weak reference may be an acceptable solution, we can't convert arguments in stubbing method calls into weak references because there are stubbing calls with an object (or a mock) that doesn't have strong ref anywhere else than arguments in Mockito.

Therefore I failed to see a solution that's transparent to callers. The possible solution below is the one that I think has the least change.

The possible solution is we can somehow reset the mock when their lives end, which clears the stubbing records. Maybe we can tie their lives to a MockitoSession. We can track all mocks created after a session is created, and reset them when the session is finished. Now we only have events for mock creation in the same thread, we may need to expand that to include other threads.

We may be able to set spiedInstance to null when their lives end, which should also be able to fix #1532 and #1533. That would be a solution with least risk.

Of course any solution that's transparent to callers is still more desirable.

A Detailed Example

See GitHub project MockitoMethodCallMemLeak. One can open it in IntelliJ and the run configuration is already configured (with mem dump at OOM and 4M java heap size).

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Feb 9, 2019

Thanks for the detailed explanation!

We may be able to set spiedInstance to null when their lives end, which should also be able to fix #1532 and #1533. That would be a solution with least risk.

Sounds like the best plan forward. Do you mind opening a PR with regression test to get that fixed?

Loading

@ttanxu
Copy link
Contributor Author

@ttanxu ttanxu commented Feb 12, 2019

It seems to be rather hard to convert all listeners to listen to events from all threads due to current test setup.

However it's pretty important to capture mocks from other threads to make this solution work, because JUnit has failOnTimeout impl which basically runs each individual test case in its own thread. That means we need start a session in each test case, rather than in setUp and tearDown, which is rather inconvenient.

I'll add a global listener that captures all mock events from all threads, but disable the feature unless caller explicitly enables in session builders.

It would be a hard refactor to make ThreadsRunAllTestsHalfManualTest pass with this.

Loading

@ttanxu
Copy link
Contributor Author

@ttanxu ttanxu commented Feb 12, 2019

Actually we probably shouldn't refactor those tests, since they probably represent part of current API. But it should be just OK to stick to the plan in my previous comment.

Loading

ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 12, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their lifespan, and clean
up all internal strong references associated with them.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 12, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their lifespan, and clean
up all internal strong references associated with them.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 12, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 12, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

User can call MockitoSessionBuilder#trackAndCleanUpMocks() to let the
session track and clean up mocks. It also relies on implementation of
mock makers to cleanUpMock(Object). SubclassByteBuddyMockMaker is
a intentional no-op to avoid unnecessary behavior changes in stable
features.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 12, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

User can call MockitoSessionBuilder#trackAndCleanUpMocks() to let the
session track and clean up mocks. It also relies on implementation of
mock makers to cleanUpMock(Object). SubclassByteBuddyMockMaker is
a intentional no-op to avoid unnecessary behavior changes in stable
features.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 12, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

User can call MockitoSessionBuilder#trackAndCleanUpMocks() to let the
session track and clean up mocks. It also relies on implementation of
mock makers to cleanUpMock(Object). SubclassByteBuddyMockMaker is
a intentional no-op to avoid unnecessary behavior changes in stable
features.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 13, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

User can call MockitoSessionBuilder#trackAndCleanUpMocks() to let the
session track and clean up mocks.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 28, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 28, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 28, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Feb 28, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

This commit also fixes mockito#1532 and fixes mockito#1533.
ttanxu added a commit to ttanxu/mockito that referenced this issue Mar 1, 2019
Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vunerable to memory leaks as
there are multiple situations where Mockito could unintentionally hold
strong references to mock instances in the map record. The strong
references could be through spiedInstance for spies, and arguments used
to faciliate method stubbing.

Mockito could never know if the arguments passed in for method stubbing
are also strongly referenced somewhere else or not, so Mockito needs to
save a strong reference to these arguments to avoid premature GC.
Therefore to solve cyclic strong references through arguments Mockito
needs to explicitly know when mocks are out of their life, and clean
up all internal strong references associated with them.

This commit also fixes mockito#1532 and fixes mockito#1533.
@mockitoguy mockitoguy closed this in 91c8e9c Mar 5, 2019
@joshallenit
Copy link

@joshallenit joshallenit commented Mar 8, 2019

Thanks! I added documentation about this as it was having a big impact on a project and was hard to find. https://github.com/mockito/mockito/wiki/What's-new-in-Mockito-2#mockito-2250

Loading

@marius-bardan
Copy link

@marius-bardan marius-bardan commented Oct 29, 2019

we just updated our testing dependencies to latest version and ran into this OOM.
how is adding an '@after'-annotated function to 3000+ tests practical, of which some already have their own implementation '@after' implementation? Or maybe I'm missing something?

Loading

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Oct 29, 2019

There was a lot of discussion about this feature in the corresponding PR: #1619. That should hopefully clear up why we were forced to implement it this way 😢

Loading

@marius-bardan
Copy link

@marius-bardan marius-bardan commented Oct 30, 2019

got it. so is there a way, that you know of, to invoke that for all tests, without me going into every test and adding that code? Maybe via some Test Listener or reflection?

Loading

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Oct 30, 2019

You can write your own test runner that extends from the default Mockito runner and implements the logic.

We might actually consider that as part of the default mockito runner implementation. Do you mind opening a PR to start that discussion? It will mostly revolve around potential issues with multithreaded code.

Loading

dahlstrom-g referenced this issue in JetBrains/intellij-community Dec 8, 2019
…anager class cannot be final

Close #1226, Close #1228

GitOrigin-RevId: 5404091a287083aaa4856a7261908876efc3486e
@joshallenit
Copy link

@joshallenit joshallenit commented Jun 1, 2020

we just updated our testing dependencies to latest version and ran into this OOM.
how is adding an '@after'-annotated function to 3000+ tests practical, of which some already have their own implementation '@after' implementation? Or maybe I'm missing something?

Classes can have as many @After functions as they want. In my case we had a base class that all classes inherited from.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants