Community
Participate
Working Groups
In https://bugs.openjdk.java.net/browse/JDK-8269232 we've discovered that Eclipse's implementation of the JDI spec does not seem to be conforming to spec. The Javadoc of ObjectReference.disableCollection() says[1]: """ Calls to this method are counted. Every call to this method requires a corresponding call to enableCollection() before garbage collection is re-enabled. """ ... while the implementation *doesn't* seem to be doing such counting. See: https://github.com/eclipse/eclipse.jdt.debug/blob/9d267305a774f5db042c71412370c915e83208f4/org.eclipse.jdt.debug/jdi/org/eclipse/jdi/internal/ObjectReferenceImpl.java#L99..L108 https://github.com/eclipse/eclipse.jdt.debug/blob/9d267305a774f5db042c71412370c915e83208f4/org.eclipse.jdt.debug/jdi/org/eclipse/jdi/internal/ObjectReferenceImpl.java#L114..L123 Contrast this to the implementation in OpenJDK: https://github.com/openjdk/jdk/blob/e515873f887ce4071ab4878a4bafca8eea67afea/src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java#L435..L444 https://github.com/openjdk/jdk/blob/e515873f887ce4071ab4878a4bafca8eea67afea/src/jdk.jdi/share/classes/com/sun/tools/jdi/ObjectReferenceImpl.java#L447..L461 We believe this to be not conforming to the JDI spec and should get fixed. [1] https://docs.oracle.com/en/java/javase/16/docs/api/jdk.jdi/com/sun/jdi/ObjectReference.html#disableCollection()
@Julian, Can you look into this?
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.debug/+/182931
@Julian, Can you spend some time in RC1?
(In reply to Sarika Sinha from comment #3) > @Julian, > Can you spend some time in RC1? The patch has been ready for review for a while ;)
(In reply to Julian Honnen from comment #4) > (In reply to Sarika Sinha from comment #3) > > @Julian, > > Can you spend some time in RC1? > The patch has been ready for review for a while ;) Thanks :) Can you please respond to the comments on the gerrit.
ping!
Created attachment 288317 [details] Gerrit patch Important discussions from Gerrit: both methods were not synchroniued before. Now they are, and this adds a possibility for deadlocks. Would it make more sense to make only read/write access to the flag synchronized? Julian Honnen Patchset 2 Jul 12, 2021 they are synchronized in the JDK implementation so I chose to do the same Simeon Andreev Patchset 2 Jul 12, 2021 I don't see how this code can deadlock. It would imply the request to the JVM can result in a callback to Eclipse, during the request. I don't think a callback is possible? And if it were, I believe the request here is just a message in a queue in the JVM; if a callback is possible, it should not occur during this method call. Is this just usual multi-threading paranoia, or am I missing something? As for making only the access to fDisableCollectionCounter synchronized and the rest of the method not synchronized, if we are going for this, fDisableCollectionCounter can be made AtomicBoolean removing the synchronized keyword altogether. Having only fDisableCollectionCounter synchronized feels like it could cause trouble, though I'm having difficulty constructing a case. At least one that follows disable/work/enable pattern. I am worried about Eclipse code that only calls enable collection, but I don't think any sort of synchronization here will help with that. So nothing this patch can help with. E.g.: 1. Thread 1 calls disable collection, code that will do work and finally enable collection is pending. 2. Thread 2 calls *only* enable collection (e.g. some clean-up code before disconnect/terminate). 3. Thread 1 runs into a GC exception during its work code. Again, nothing the patch here can actually fix. Andrey Loskutov Patchset 2 Jul 18, 2021 The lock here is on the entire ObjectReferenceImpl object. So if someone else *in Eclipse* synchronizes something via the same lock already and waits on this thread, we will have a problem. Also the code that runs after the requestVM (error handler that catches the possible exception we may throw) may do something now while holding the lock on this object. Yes, that is a paranoia :-) Not sure one can simply use AtomicLong or use the internal lock object here (not accessible to anyone outside this class).
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
