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
Conversation
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 |
(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!) |
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 Report
@@ 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
Continue to review full report at Codecov.
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thanks to @mockitoguy . Just some context here. I tried to avoid new API at first, but I didn't know how much impact there would be to add a new method in I hope the review won't later become a whole refactor of current implementation. |
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): 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! |
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 I also first tried to avoid 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:
Then it basically treats the life of any |
Oh... I already added |
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. |
As a sidenote: the refactoring with the new InlineMockMaker interface is 👍 for me 😄 |
Hey @ttanxu, so you you're saying that the approach I suggested in a) wouldn't work? |
Great! I'll review again this weekend. |
@TimvdLippe, I'm with you. It's a good idea to have a separate interface and stay compatible. |
Hey @mockitoguy , what you suggested could solve the same issue that is solved by
|
@ttanxu, thank you for explanation. I'm looking into it. |
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? |
Correct me if I'm wrong. It seems you're suggesting one If we still only have one |
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? |
Oh... You say we don't have to save created mocks in a list in Removing |
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 With that said I'll change |
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? |
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?
Do we know legit use cases for "static" mocks? It seems an anti-pattern. |
c1ad3a0
to
f464f7a
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely looking better, nice work! Just some small questions
@@ -271,6 +272,15 @@ public void resetMock(Object mock, MockHandler newHandler, MockCreationSettings | |||
} | |||
} | |||
|
|||
@Override | |||
public void cleanUpMock(Object mock) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated them for consistency. Thanks.
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.
Reviewing... |
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! |
I will review this on Monday in the office. Thanks for the polish! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* See example in {@link MockitoFramework#clearInlineMocks()}. | |
* See example usage in {@link MockitoFramework#clearInlineMocks()}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
* | ||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 I'm fixing it now.
@ttanxu, great point! |
I'm making the final tweaks based on the code review and I'm merging. |
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 😉 |
Hi, Just to let you know I am using the new API already with v2.28.2 and for me it was breaking the Strict test with following exception:
I had to move it to |
@s2131 You are not allowed to interact with mocks anymore after calling that API because all internal states of mocks are cleaned up after the API call. That includes all delayed actions or interactions from other threads. Unfortunately it's never as easy as just calling that API. |
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!):
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.