Fix StackOverflowError with AbstractList after using mockSingleton#3790
Fix StackOverflowError with AbstractList after using mockSingleton#3790raphw merged 2 commits intomockito:mainfrom
Conversation
cd85319 to
91b6035
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3790 +/- ##
============================================
+ Coverage 86.50% 86.83% +0.33%
- Complexity 3004 3012 +8
============================================
Files 343 341 -2
Lines 9099 9095 -4
Branches 1121 1124 +3
============================================
+ Hits 7871 7898 +27
+ Misses 945 908 -37
- Partials 283 289 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
raphw
left a comment
There was a problem hiding this comment.
This is a good guardrail. Could I asked you to add the same mechanism in mockedStatics? It might call a hashCode that delegates to a static utility, what would cause the same issue, so we should handle this consistently.
|
Actually I think it's not susceptible to the same issue because for |
|
Indeed. Would need a custom hashCode method in a future JVM that calles a static method outside of |
|
Ah, I think we should avoid the weak hash map here: https://github.com/mockito/mockito/blob/main/mockito-core/src/main/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMaker.java#L698 If the hashCode/equals method is broken this will yield weird results, and that even more if two equal singleton objects exist. I would go for an |
|
With this, there should never be calls on the object itself and the guardrail is no longer needed. |
raphw
left a comment
There was a problem hiding this comment.
Avoid use of methods on singleton objects and use identity reference.
|
I am wondering if the weak reference semantics are still important for this impl. I think for "singletons" the weak ref behavior doesn't really matter. For Java enums at least, they should never be GCed. |
91b6035 to
432b2f2
Compare
raphw
left a comment
There was a problem hiding this comment.
I think the weak reference is relevant here. Not often, but in some cases it can drag an entire class loader with it which would otherwise be collected, causing memory leaks in case that a single test does not properly close the mocking context.
In order to implement a proper weak map, you need to register s reference queue and check for new items on it. Those entries can then be removed from the map. Otherwise, you need to keep the system hash code stored in the custom reference and compare by identity. If a weak reference is null it cannot be equal any more as it's been collected.
|
I'm wary of building a new map implementation from scratch to satisfy both weakness and identity. I found some discussion on https://stackoverflow.com/questions/22910375/combo-of-identityhashmap-and-weakhashmap which points to Guava or https://github.com/plume-lib/hashmap-util/blob/main/src/main/java/org/plumelib/util/WeakIdentityHashMap.java (unclear how battle-tested this is). I guess neither are really viable options,. It seems to me a recursion guard (like the original commit) is simpler to maintain even though it's theoretically incomplete. What do you think @raphw ? |
|
I think we need to support identity and weak references, we cannot choose as lacking either will lead to subtle bugs, and with Mockito's reach users will run into those. In our case, I would like to avoid the dependency, as Guava can quickly lead to conflicts. Wrapping the mocks seems like a realistic goal as we do not expose the map as general purpose. I believe that is the solution we should aim for. |
Replace WeakHashMap with a WeakIdentityMap that uses System.identityHashCode and == for key comparison. This avoids calling instrumented hashCode()/equals() methods on mock instances during map lookups, which caused infinite recursion when mocking classes like AbstractList whose hashCode() invokes instrumented methods.
|
It seems that |
432b2f2 to
6707f78
Compare
raphw
left a comment
There was a problem hiding this comment.
Last nit pick: add a note to the exception that states that the instance is already mocked that this is for the current thread, to make it obvious.
| * lookups. Hash collisions (distinct objects sharing an identity hash code) are resolved by | ||
| * linear scan within the bucket using {@code ==}. Lookups do not allocate. | ||
| */ | ||
| public class WeakIdentityMap<V> extends AbstractMap<Object, V> { |
There was a problem hiding this comment.
Nitpick: we do not need to implement a full map API for the needs we have, but it does neither hurt, and since it is internal API, we can remove or change this API later anyways. We can keep this for this reason, and it makes the code easier to comprehend, possibly.
(I noticed this issue when integrating with Mockito-Kotlin and running its test suite)
WeakHashMap.get()callshashCode()on its keys, which for instrumented classes triggers the byte buddy advice, callingisMocked() -> getSingletonMockInterceptor() -> WeakHashMap.get()recursively.This adds a thread-local recursion guard around the
WeakHashMap.get()call.Alternatively, we could use a different map implementation like IdentityHashMap which doesn't call
hashCode()but I didn't want to diverge frommockStaticwhich also uses aWeakHashMap(though I'm not convinced the weak semantics are important)