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

New API to clean up all inline mocks after test #1619

Merged
merged 10 commits into from Mar 5, 2019

Conversation

Projects
None yet
5 participants
@ttanxu
Copy link
Contributor

ttanxu commented Feb 12, 2019

In certain specific, rare scenarios (issue #1614) 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!):

public class ExampleTest {

     @After
     public void clearMocks() {
         Mockito.framework().clearInlineMocks();
     }

     @Test
     public void someTest() {
         //...
     }
 }

Due to the introduction of map from weak reference from mock instance to
its invocation handler, Mockito became vulnerable 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 facilitate 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.

Fixes #1532 and #1533.

@ttanxu ttanxu force-pushed the ttanxu:release/2.x branch from 59efe50 to 5cf0331 Feb 12, 2019

@TimvdLippe TimvdLippe self-requested a review Feb 12, 2019

@TimvdLippe

This comment has been minimized.

Copy link
Contributor

TimvdLippe commented Feb 12, 2019

I will review this tomorrow. At a first glance, we are going to need to make some changes. Most notably the way we handle interfaces. (A new method on MockMaker is a breaking change). Will give you a detailed review tomorrow.

@TimvdLippe

This comment has been minimized.

Copy link
Contributor

TimvdLippe commented Feb 12, 2019

(That said, the PR is much appreciated. Sorry if my initial reaction seemed not positive, we really do appreciate community PRs for these kind of issues!)

@ttanxu ttanxu force-pushed the ttanxu:release/2.x branch from 5cf0331 to 9343bec Feb 12, 2019

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 12, 2019

Oh... Thanks for quick response. I didn't expect any feedback today.

I am not familiar with how to contribute to Mockito (i.e. don't know what can be done, and what can't). It's expected to have some back and forth for a change at this scale. Just please be as detailed as possible, so that I know how to make proper changes.

I just hope the overall approach won't be vetoed, as I basically don't have any other ideas on how to solve it.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 12, 2019

Codecov Report

Merging #1619 into release/2.x will increase coverage by 0.02%.
The diff coverage is 85.71%.

Impacted file tree graph

@@                Coverage Diff                @@
##             release/2.x    #1619      +/-   ##
=================================================
+ Coverage          87.38%   87.41%   +0.02%     
- Complexity          2435     2442       +7     
=================================================
  Files                301      301              
  Lines               6278     6285       +7     
  Branches             784      784              
=================================================
+ Hits                5486     5494       +8     
  Misses               596      596              
+ Partials             196      195       -1
Impacted Files Coverage Δ Complexity Δ
...l/creation/bytebuddy/InlineByteBuddyMockMaker.java 68.42% <100%> (+3.58%) 14 <2> (+3) ⬆️
...to/internal/framework/DefaultMockitoFramework.java 89.47% <80%> (-10.53%) 10 <5> (+5)
...ternal/util/reflection/GenericMetadataSupport.java 86.86% <0%> (-0.27%) 38% <0%> (ø)
...aultanswers/RetrieveGenericsForDefaultAnswers.java 95.91% <0%> (-0.24%) 20% <0%> (-1%)
src/main/java/org/mockito/Mockito.java 96.61% <0%> (ø) 40% <0%> (ø) ⬇️
...rnal/creation/bytebuddy/MockMethodInterceptor.java 72.72% <0%> (ø) 5% <0%> (ø) ⬇️
...rnal/stubbing/defaultanswers/ReturnsDeepStubs.java 100% <0%> (+2.17%) 16% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af2d33b...959cba4. Read the comment docs.

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Feb 13, 2019

Thank you for the PR! Since this is changing / adding new public API I will review it, too (many thanks to @TimvdLippe for offering to review this!). I'll commit to provide review by the end of this week.

@TimvdLippe
Copy link
Contributor

TimvdLippe left a comment

I will let @mockitoguy do the full review, as he has been working on the listeners. So far I have just 1 comment regarding the MockMaker, but that should be easy to fix hopefully.

Show resolved Hide resolved src/main/java/org/mockito/plugins/MockMaker.java Outdated
@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 13, 2019

Thanks to @mockitoguy .

Just some context here. I tried to avoid new API at first, but DefaultMockitoSession seems (at least from tests) to be used within a thread, and changing its default behavior to listen on events from all threads will break tests for multi-threading. Therefore I came up with the new API solution.

I didn't know how much impact there would be to add a new method in MockMaker interface. That requires everyone providing an implementation when they uprev their Mockito to a version that has the new method, but I have no clue how much work it could be. If it's too noisy maybe we can have a default impl (if only Java 8+ is supported) or a brand new interface called InlineMockMaker to add that method. I personally want to expose a chance to mock makers to clean up as we don't know how they handle the mapping from mock to its handler, which may have other places that could cause memory leaks other than those 2 places we found. We need better documentation on that, but I have to admit I'm not good at writing API docs so I'll defer to any of your suggestions in terms of documentation.

I hope the review won't later become a whole refactor of current implementation.

@ttanxu ttanxu force-pushed the ttanxu:release/2.x branch from 9343bec to cbc1101 Feb 13, 2019

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Feb 18, 2019

I reviewed the implementation and most of it is really good. It is also a very high quality PR, thank you! Curious, are you facing this problem at G.?

I have a couple of concerns / design choices to discuss:

a) Can we make the fix more seamless? I'm not sure if this is possible but let's discuss. Goals: users don't have to remember to use trackAndCleanUpMocks() when inline mocker is in use. For example (brainstorming):
- we introduce a new interface that InlineByteBuddyMockMaker implements, say: RequiresCleaningMocks
- we expose a new method like framework.getPlugins().getPlugin(MockMacker.class), this way we can find out in runtime what MockMaker is in use.
- in Mockito Session, we check if the mock maker is RequiresCleaningMocks
- in session, we call RequiresCleaningMocks.cleanUpMock()

b) MockMaker.cleanUpMock() is an incompatible change so we cannot ship this PR in its current form. MockMaker is typically provided at runtime from a different Jar (example: dexmaker, powermockito). I have a couple ideas but first let's explore a)

Great tests, deep change in the internals of Mockito, thank you very much for the contribution!

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 19, 2019

Thanks for your review @mockitoguy .

Dexmaker is incubating a feature in inline-dexmaker to create spies without creating new instances, so that we can stub final members, and we (Android framework) found these mem leaks when using that. I then found #1533 and reproduced #1614 that I confirmed it also happens with ByteBuddy mock makers so I started the fix with help from @TimvdLippe .

First of all I really didn't see a solution w/o tying mock lives with something, and I think MockitoSession is the most natural choice for the "something". So technically it's already not transparent to properly use inline mock makers because sessions are necessary to make it work while it's optional to use subclass mock makers.

I also first tried to avoid trackAndCleanUpMocks(). The difficulty actually comes from the fact MockitoListeners are ThreadLocal before this change, and it's a requirement to track mocks generated from other threads because FailOnTimeout basically runs tests in different threads than the thread used to run Before and After. Before this change user can have multiple DefaultMockitoSession, as long as they are in different threads (ThreadsRunAllTestsHalfManualTest also verifies that behavior) and not interfering with each other. However if we convert DefaultMockitoSession to have "thread global" influence ThreadRunAllTestsHalfManualTest will fail because cross-threads mock cleanup will invalidate mocks created in tests running in a different thread.

I think I have a way (a bit hacky though) to make it work, but I don't think its complication outweighs the new API (given that it's already not transparent to users). It doesn't encourage clean tests in multi-threading scenario either imho. However I can still put my thought here and let you decide if we should take it:

  • Don't reject registration of global listeners if it's from a different thread for the same type
  • Only clean up mocks when the last MockTracker is unregistered

Then it basically treats the life of any DefaultMockitoSession as mock lives. It doesn't allow user to precisely control mock lives but with some care users should still be able to avoid OOM.

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 19, 2019

Oh... I already added InlineMockMaker interface so not all mock makers need to implement cleanUpMock(). I'm not sure if the plugin way is still preferred over current change because for most dexmakers they're still just implementing the basic MockMaker interface and nothing should break. While they can choose to implement InlineMockMaker if clean up is necessary.

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 21, 2019

Just to be clear that I'm waiting for @mockitoguy 's response on if we would like to go for that approach in exchange of removing trackAndCleanUpMocks() API, just in case we're waiting for each other and it's going nowhere.

@TimvdLippe

This comment has been minimized.

Copy link
Contributor

TimvdLippe commented Feb 21, 2019

As a sidenote: the refactoring with the new InlineMockMaker interface is 👍 for me 😄

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Feb 24, 2019

Hey @ttanxu, so you you're saying that the approach I suggested in a) wouldn't work?

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Feb 24, 2019

Oh... I already added InlineMockMaker interface so not all mock makers need to implement cleanUpMock()

Great! I'll review again this weekend.

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Feb 24, 2019

As a sidenote: the refactoring with the new InlineMockMaker interface is 👍 for me 😄

@TimvdLippe, I'm with you. It's a good idea to have a separate interface and stay compatible.

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 24, 2019

Hey @mockitoguy , what you suggested could solve the same issue that is solved by InlineMockMaker in current PR (the MockMaker contract breakage).

trackAndCleanUpMocks() was introduced for other reason, but could use different tech to remove its necessity. I explained the detail in previous comments (mostly because of the multi-thread implementation of FailOnTimeout in JUnit).

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Feb 25, 2019

@ttanxu, thank you for explanation. I'm looking into it.

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Feb 27, 2019

My comment is short but I put a lot of consideration into this. Given all arguments, I agree that a new API is the best way. Before we ship it, I want to bounce one more idea. What if we tie MockMaker with MockitoSession? When the session completes it tears down its MockMaker/cleans up all mocks. We could use ThreadLocal. Thoughts?

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 27, 2019

Correct me if I'm wrong.

It seems you're suggesting one MockMaker per thread. The critical part of this approach is the mock map in the inline mock maker needs to be a ThreadLocal, which means mocks won't look have handlers tied to them in threads other than the thread where they were created, and thus won't appear to be a mock and method invocations on it might trigger exceptions. That's not likely a compatible change to existing use cases because if FailOnTimeout is used Befores and Afters are executed in a different thread than the test method.

If we still only have one MockMaker across all threads then the structure is pretty much the same as current change except that instead of calling InlineMockMaker#clearUp() we just tear down the entire MockMaker. I'm not sure if this is better than calling clearUp(). It does sound like a viable solution w/o introduction to InlineMockMaker as we can just swap the instance of MockMaker in PluginRegistry. The con is that's more of a systematic refactor than InlineMockMaker#clearUp() approach and it reduces the cleanness of current code (we can't use final for mockMaker in PluginRegistry anymore). BTW this doesn't help avoid a new API trackAndClearUpMocks() in MockitoSession. I'll leave this decision to you.

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Feb 28, 2019

Great points, thank you for taking the time to explain. Why do we need to keep track of specific mocks? Can we just clear all?

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 28, 2019

Oh... You say we don't have to save created mocks in a list in MockTracker? I guess we can just clear all mocks. Especially considering there can't be multiple mock-tracking sessions open at the same time, mocks created before the session is opened may be leaked.

Removing MockTracker can simply this change a lot, especially we don't need global listeners. I'll update the change.

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 28, 2019

Actually, I recall that the thought of tracking specific mocks stemmed from the thought that usually mocks have different lives. E.g. mocks held by static variables, created in BeforeClass, in Before and in individual tests may need to have different lives. It would be nice if we can keep mocks created in BeforeClass for later reuse.

Current impl is half baked because there can only be one tracking session, but I want to keep cleanUp(Object) signature (or we can have cleanUp(Object...) instead so that we can clean up everything if Object array is null) for InlineMockMaker just in case we want to support the case above.

With that said I'll change cleanUp(Object) to cleanUp(Object...) and use null array for clean up everything.

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 28, 2019

Oh... Besides, if we just want to clear everything, it doesn't seem natural to me to add the API to MockitoSession, maybe we can add an API to Mockito?

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Feb 28, 2019

Thank you for considering! I want us to strive for simplicity. I'll think about the new API for Mockito (or MockitoFramework).

If we have a static method like "Mockito.framework().clearAllMocks()" would it work for you?

E.g. mocks held by static variables, created in BeforeClass, in Before and in individual tests may need to have different lives

Do we know legit use cases for "static" mocks? It seems an anti-pattern.

@ttanxu ttanxu force-pushed the ttanxu:release/2.x branch 2 times, most recently from c1ad3a0 to f464f7a Feb 28, 2019

@ttanxu ttanxu force-pushed the ttanxu:release/2.x branch 2 times, most recently from a2fc2d6 to 223c222 Feb 28, 2019

@ttanxu

This comment has been minimized.

Copy link
Contributor Author

ttanxu commented Feb 28, 2019

Finally cleaned up all remains from previous change. It's a lot simpler now. Please take a look.

I can think of a case for static mocks where constructing a mock may be expensive and someone wants to reuse it across tests. That's not an argument why static mocks have to be used though, they can definitely reconstruct those mocks.

@TimvdLippe
Copy link
Contributor

TimvdLippe left a comment

This is definitely looking better, nice work! Just some small questions

Show resolved Hide resolved src/main/java/org/mockito/plugins/InlineMockMaker.java Outdated
@@ -271,6 +272,15 @@ public void resetMock(Object mock, MockHandler newHandler, MockCreationSettings
}
}

@Override
public void cleanUpMock(Object mock) {

This comment has been minimized.

@TimvdLippe

TimvdLippe Feb 28, 2019

Contributor

We are never calling this method with an actual mock. Is there a situation this should be the case? Could you maybe provide an example for it? (If it is not necessary, we might as well make this method clearAllMocks as well)

This comment has been minimized.

@ttanxu

ttanxu Mar 1, 2019

Author Contributor

I said that in comments before I updated the PR.

If we will go with clearAllMocks() we need an additional method if there is strong feature request for keeping some mocks alive later.

That might be over-engineering, but I'm not sure how open you are to introduce new method to incubating class or change existing incubating method signature in this case. It would require mock maker changes later.

Maybe we can change the public API to take in mocks as well? I might just do that quickly.

This comment has been minimized.

@ttanxu

ttanxu Mar 1, 2019

Author Contributor

Updated the PR, but I'm open to discuss this forward. If you two believe cutting off the mock specific clear up feature is better then I'm willing to make another change.

I'm just afraid that there are cases where callers might want to keep some mocks, but still need to release some other of them.

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 1, 2019

Contributor

I think it is fair to keep both. Since we are introducing an interface, we are most flexible if we keep them both in. That said, I think it is better to also introduce clearAllMocks on InlineByteBuddyMockMaker as well as rename cleanUpMock to clearMock to keep it consistent.

This comment has been minimized.

@ttanxu

ttanxu Mar 1, 2019

Author Contributor

Updated them for consistency. Thanks.

@ttanxu ttanxu force-pushed the ttanxu:release/2.x branch from 223c222 to 85ea8eb Mar 1, 2019

Fixes #1614: Add API to clean up mocks.
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 #1532 and fixes #1533.

@ttanxu ttanxu force-pushed the ttanxu:release/2.x branch from 85ea8eb to 91c8e9c Mar 1, 2019

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Mar 2, 2019

Reviewing...

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Mar 2, 2019

We are ready. Thank you @ttanxu and @TimvdLippe for patience! This is the best pragmatic step to resolve the problem. In the future, perhaps we can avoid having the weak map of references in a first place.

@ttanxu, I made some tweaks to tests, documentation, I've also renamed the method to "clearMocks() -> clearInlineMocks()", for consistency. Take a look and see if you're OK with this. Let us know and I'll merge this. Nice work with this change!

@TimvdLippe, I did some cosmetic changes, please review if you want. Thanks!

@TimvdLippe

This comment has been minimized.

Copy link
Contributor

TimvdLippe commented Mar 2, 2019

I will review this on Monday in the office. Thanks for the polish!

@TimvdLippe
Copy link
Contributor

TimvdLippe left a comment

Some small documentation changes. Also, I think we should do this eagerly in the JUnit runner and extension. I don't see a situation where this is undesirable to do.

* <p>
* This method is useful to tackle subtle memory leaks that are possible due to the nature of inline mocking
* (issue <a href="https://github.com/mockito/mockito/pull/1619">#1619</a>).
* If you are facing those problems, call this method at the end of the test (or in "@After" method).

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 4, 2019

Contributor

Shouldn't we update our JUnit runner and extension to handle this automatically? I think this is an improvement in any situation, so we can just go ahead and do it eagerly.

This comment has been minimized.

@mockitoguy

mockitoguy Mar 4, 2019

Member

This would be a breaking change in thread-based parallel tests that are supported by Mockito (with limitations, as we stated in the FAQ).

If you turn this on by default, some Mockito tests will start failing. @ttanxu, discussed this somewhere in this PR (but the thread became long :).

We can always make it later if we understand the use patterns better. I suggest we expose the this API first and observe / learn how it goes.

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 4, 2019

Contributor

Ah okay, I missed that discussion. Thanks for the clarification.

* 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 in {@link MockitoFramework#clearInlineMocks()}.

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 4, 2019

Contributor
Suggested change
* See example in {@link MockitoFramework#clearInlineMocks()}.
* See example usage in {@link MockitoFramework#clearInlineMocks()}.

This comment has been minimized.

@mockitoguy
*
* In certain specific, rare scenarios (issue <a href="https://github.com/mockito/mockito/pull/1619">#1619</a>)
* inline mocking causes memory leaks.
* There is no clean way to mitigate this problem completely.

This comment has been minimized.

@TimvdLippe

TimvdLippe Mar 4, 2019

Contributor
Suggested change
* There is no clean way to mitigate this problem completely.
* It was not possible to resolve this issue without introducing an additional API.
* Any inline MockMaker should therefore implement the additional interface {@link org.mockito.plugins.InlineMockMaker} to allow Mockito to properly clean up mocks.

This comment has been minimized.

@mockitoguy

mockitoguy Mar 5, 2019

Member

I'm skipping this suggestion if that's OK. I prefer skipping those details in the high level javadoc. Also, I prefer the original text grammatically.

Feel free to redo the javadoc if you want, in a separate PR - I'm happy to review/merge it.

@ttanxu
Copy link
Contributor Author

ttanxu left a comment

Generally looks good, just one comment. I suppose I don't have to address @TimvdLippe 's comment this time, since they're all about documentation (which I am not good at).

BTW, there is a conflict in version.properties.

@Test
public void clears_all_mocks() {
//clearing mocks only works with inline mocking
assumeTrue(Plugins.getMockMaker() instanceof InlineMockMaker);

This comment has been minimized.

@ttanxu

ttanxu Mar 4, 2019

Author Contributor

FWIW, one of the reason why I didn't use assume at first is that it can test user can safely call clearInlineMocks() and clearMock() even when mock maker is not a inline mock maker.

That's not very important I suppose, but still would be good to cover that in tests imo.

This comment has been minimized.

@mockitoguy

mockitoguy Mar 5, 2019

Member

+1 I'm fixing it now.

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Mar 5, 2019

test user can safely call clearInlineMocks() and clearMock() even when mock maker is not a inline mock maker

@ttanxu, great point!

@mockitoguy

This comment has been minimized.

Copy link
Member

mockitoguy commented Mar 5, 2019

I'm making the final tweaks based on the code review and I'm merging.

@mockitoguy mockitoguy changed the title Fixes #1614: Clean up mocks when session finishes. New API to clean up all inline mocks after test Mar 5, 2019

@mockitoguy mockitoguy merged commit a7ee084 into mockito:release/2.x Mar 5, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@ming13

This comment has been minimized.

Copy link

ming13 commented Mar 5, 2019

This is awesome, thank you guys! I’ll try this API on our codebase and samples I’ve provided in original issues. Also — kudos for including tests reproducing the behavior I’ve described 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.