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

JDIType.throwDebugException() doesn't properly handle disconnected exception #192

Closed
iloveeclipse opened this issue Mar 8, 2023 · 0 comments · Fixed by #193
Closed

JDIType.throwDebugException() doesn't properly handle disconnected exception #192

iloveeclipse opened this issue Mar 8, 2023 · 0 comments · Fixed by #193
Assignees
Labels
bug Something isn't working
Milestone

Comments

@iloveeclipse
Copy link
Member

In our automated tests we see sometimes logged errors like:

junit.framework.AssertionFailedError: 1 unexpected error(s) or warning(s) found in the log. Messages: [Internal error logged from JDI Debug: ]. First error or warning: Internal error logged from JDI Debug: . 
Stack:com.sun.jdi.VMDisconnectedException: Got IOException from Virtual Machine
	at org.eclipse.jdi.internal.connect.PacketSendManager.sendPacket(PacketSendManager.java:93)
	at org.eclipse.jdi.internal.MirrorImpl.requestVM(MirrorImpl.java:190)
	at org.eclipse.jdi.internal.MirrorImpl.requestVM(MirrorImpl.java:230)
	at org.eclipse.jdi.internal.MirrorImpl.requestVM(MirrorImpl.java:262)
	at org.eclipse.jdi.internal.ClassTypeImpl.superclass(ClassTypeImpl.java:309)
	at org.eclipse.jdt.internal.debug.core.model.JDIClassType.getSuperclass(JDIClassType.java:145)

The test doesn't expect that error in the log and fails (but would succeed without error log entry).
The error itself is unconditionally logged by the framework even if the debug listener works just fine & properly handles the case if the debug target is gone.

The error is logged because in JDIDebugElement.logError()

the getDebugTarget().isAvailable() says "yes, I'm still there" even if JVM is already disconnected.

The root cause of it is a subtle difference between JDIType.throwDebugException() and JDIDebugElement.throwDebugException(): the overridden method in JDIType doesn't call JDIDebugElement.disconnected(), so after JDIType.throwDebugException() is called above with VMDisconnectedException in JDIClassType.getSuperclass() debug target is still assuming that JVM is connected.

protected void throwDebugException(String message, int code,

The problem was introduced long time ago:

  • The original JDIDebugElement code was copy/pasted to JDIType and at that time original code did not had yet disconnected() call, see this commit.
  • Same year later the original code in JDIDebugElement was "cleaned up" and disconnected() call was added, but only in original code, not in the copied code, see this commit.
  • One year later JDIType refactoring happened where JDIType extended JDIDebugElement see this commit but the method was already there and nobody noticed the small difference.

What we learn: copy/paste is root of all evil.

I will provide a patch.

@iloveeclipse iloveeclipse added the bug Something isn't working label Mar 8, 2023
@iloveeclipse iloveeclipse added this to the 4.28 M1 milestone Mar 8, 2023
@iloveeclipse iloveeclipse self-assigned this Mar 8, 2023
iloveeclipse added a commit to iloveeclipse/eclipse.jdt.debug that referenced this issue Mar 8, 2023
iloveeclipse added a commit to iloveeclipse/eclipse.jdt.debug that referenced this issue Mar 8, 2023
exception (eclipse-jdt#192)

TL;DR: simply don't override the super method - it does already the
"right thing".

See longer explanation in eclipse-jdt#192.

Fixes eclipse-jdt#192
iloveeclipse added a commit that referenced this issue Mar 8, 2023
iloveeclipse added a commit that referenced this issue Mar 8, 2023
exception (#192)

TL;DR: simply don't override the super method - it does already the
"right thing".

See longer explanation in #192.

Fixes #192
SarikaSinha added a commit that referenced this issue Mar 11, 2023
* Catch and log exceptions from IJDIEventListener during event dispatch

IF IJDIEventListener.handleEvent() throws an exception during
EventDispatcher.dispatch(), the debugger breaks. Stepping, resuming,
terminating, etc. doesn't work.

This change adds a try-catch block around
IJDIEventListener.handleEvent() in EventDispatcher.dispatch(), to ensure
faulty listeners don't break the debugger.

Fixes: #172

* Disable GC for exception in ExceptionEvent until event is processed

This change disables garbage collection for
ExceptionEventImpl.fException, until the event is processed by listeners
in EventDispatcher.dispatch().

Fixes: #167

* Check if AskRecurrenceDialog was cancelled and has null result (#176)

Fixes #176

* Remove references to deprecated Navigator in Scrapbook editor

It's up for removal thus offering it makes no sense.

* Clean up duplicated code in JavaRuntime

Replace the code checking if a classpath entry is modular with the new
ClasspathEntry.isModular().

Fixes: #184

* POM and product version changes for 4.28 release

Fixes eclipse-platform/eclipse.platform.releng.aggregator#916

* Bump bundle version for 4.28 (#192)

See #192

* JDIType.throwDebugException() doesn't properly handle disconnected
exception (#192)

TL;DR: simply don't override the super method - it does already the
"right thing".

See longer explanation in #192.

Fixes #192

---------

Co-authored-by: Simeon Andreev <simeon.danailov.andreev@gmail.com>
Co-authored-by: Andrey Loskutov <loskutov@gmx.de>
Co-authored-by: Александър Куртаков <akurtakov@gmail.com>
Co-authored-by: Rahul Mohanan <rahul.mohanan@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
1 participant