Conversation
📝 WalkthroughWalkthroughThis PR upgrades the Espressif IDF Eclipse Plugin GDB/OpenOCD debug bundle to Java 21 and refactors its launch lifecycle management with improved process tracking, concurrent data structures, and virtual-thread-based cancellation handling. ChangesLaunch Lifecycle and Process Management Upgrade
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bundles/com.espressif.idf.debug.gdbjtag.openocd/META-INF/MANIFEST.MF`:
- Line 27: The manifest currently sets Bundle-RequiredExecutionEnvironment to
JavaSE-21 which violates repo policy; update the MANIFEST.MF entry
Bundle-RequiredExecutionEnvironment back to JavaSE-17 and, if this bundle
defines plugin.xml extensions, ensure the manifest contains a
Bundle-SymbolicName entry with the suffix ;singleton:=true (add or adjust the
Bundle-SymbolicName key accordingly) so the bundle meets the repository's
execution-environment and singleton requirements.
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java`:
- Around line 268-278: In Launch.terminate() do not swallow failures: inside the
try/catch around the isTerminated() check and super.terminate(), after logging
the caught Exception via Logger.log(e) rethrow it as a DebugException if it
already is one (or throw a new DebugException wrapping the original Exception)
so callers see termination failures; ensure the catch distinguishes
DebugException vs other Exceptions and rethrows appropriately after logging
instead of silently returning.
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java`:
- Around line 169-170: The throw in LaunchConfigurationDelegate currently wraps
e.getCause() which can be null and loses context; change the DebugException
construction to pass the original IOException (the caught variable, likely named
e) as the nested throwable instead of e.getCause(), i.e. when throwing new
DebugException(new Status(..., "Error launching command", e.getCause())),
replace e.getCause() with e so the original exception and stack trace are
preserved.
- Around line 226-243: The code unconditionally calls
mainLaunchThread.interrupt() after the wait loop, causing spurious interrupts on
normal termination; change the logic so that mainLaunchThread.interrupt() is
only invoked when the wait ended due to cancellation (finalMonitor.isCanceled())
or when you explicitly forced termination—i.e., wrap the launch.terminate() and
mainLaunchThread.interrupt() calls in the cancellation/forced-termination branch
and do not interrupt when launch.isTerminated() returned true normally; update
the block that uses launch.canTerminate(), launch.terminate(), and
mainLaunchThread.interrupt() so the interrupt is only executed on
cancellation/forced termination and keep the existing exception handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1ebf45c-79f8-4f14-9b83-b74a40ca4a6a
📒 Files selected for processing (6)
bundles/com.espressif.idf.debug.gdbjtag.openocd/.classpathbundles/com.espressif.idf.debug.gdbjtag.openocd/.settings/org.eclipse.jdt.core.prefsbundles/com.espressif.idf.debug.gdbjtag.openocd/META-INF/MANIFEST.MFbundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.javabundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.javabundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchProcessDictionary.java
| org.eclipse.embedcdt.ui, | ||
| org.eclipse.ui.console | ||
| Bundle-RequiredExecutionEnvironment: JavaSE-17 | ||
| Bundle-RequiredExecutionEnvironment: JavaSE-21 |
There was a problem hiding this comment.
Keep required execution environment at JavaSE-17 per repo policy.
Changing to JavaSE-21 breaks the bundle-level execution-environment requirement defined for this repo and can invalidate target-platform compatibility checks.
As per coding guidelines, "Bundle MANIFEST.MF must set Bundle-SymbolicName with singleton:=true if it has plugin.xml extensions and Bundle-RequiredExecutionEnvironment: JavaSE-17".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bundles/com.espressif.idf.debug.gdbjtag.openocd/META-INF/MANIFEST.MF` at line
27, The manifest currently sets Bundle-RequiredExecutionEnvironment to JavaSE-21
which violates repo policy; update the MANIFEST.MF entry
Bundle-RequiredExecutionEnvironment back to JavaSE-17 and, if this bundle
defines plugin.xml extensions, ensure the manifest contains a
Bundle-SymbolicName entry with the suffix ;singleton:=true (add or adjust the
Bundle-SymbolicName key accordingly) so the bundle meets the repository's
execution-environment and singleton requirements.
Comment on lines
+268
to
+278
| try | ||
| { | ||
| if (!this.isTerminated()) | ||
| { | ||
| super.terminate(); | ||
| } | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| Logger.log(e); | ||
| } |
There was a problem hiding this comment.
Do not swallow termination failures in terminate().
Catching broad Exception and only logging makes termination look successful even when it fails, which can leave the launch lifecycle inconsistent. Re-throw DebugException (or wrap non-debug exceptions) after logging.
Suggested fix
try
{
if (!this.isTerminated())
{
super.terminate();
}
}
- catch (Exception e)
+ catch (DebugException e)
+ {
+ Logger.log(e);
+ throw e;
+ }
+ catch (Exception e)
{
Logger.log(e);
+ throw new DebugException(new Status(IStatus.ERROR, Activator.PLUGIN_ID,
+ IDsfStatusConstants.INTERNAL_ERROR, "Error terminating launch", e));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try | |
| { | |
| if (!this.isTerminated()) | |
| { | |
| super.terminate(); | |
| } | |
| } | |
| catch (Exception e) | |
| { | |
| Logger.log(e); | |
| } | |
| try | |
| { | |
| if (!this.isTerminated()) | |
| { | |
| super.terminate(); | |
| } | |
| } | |
| catch (DebugException e) | |
| { | |
| Logger.log(e); | |
| throw e; | |
| } | |
| catch (Exception e) | |
| { | |
| Logger.log(e); | |
| throw new DebugException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, | |
| IDsfStatusConstants.INTERNAL_ERROR, "Error terminating launch", e)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java`
around lines 268 - 278, In Launch.terminate() do not swallow failures: inside
the try/catch around the isTerminated() check and super.terminate(), after
logging the caught Exception via Logger.log(e) rethrow it as a DebugException if
it already is one (or throw a new DebugException wrapping the original
Exception) so callers see termination failures; ensure the catch distinguishes
DebugException vs other Exceptions and rethrows appropriately after logging
instead of silently returning.
Comment on lines
169
to
+170
| throw new DebugException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, DebugException.REQUEST_FAILED, | ||
| "Error while launching command: " + StringUtils.join(cmdArray, " "), e.getCause()));//$NON-NLS-2$ | ||
| "Error launching command", e.getCause())); |
There was a problem hiding this comment.
Wrap the original IOException, not e.getCause().
Using e.getCause() here often passes null and drops the real failure context.
Suggested fix
catch (IOException e)
{
throw new DebugException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, DebugException.REQUEST_FAILED,
- "Error launching command", e.getCause()));
+ "Error launching command", e));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| throw new DebugException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, DebugException.REQUEST_FAILED, | |
| "Error while launching command: " + StringUtils.join(cmdArray, " "), e.getCause()));//$NON-NLS-2$ | |
| "Error launching command", e.getCause())); | |
| catch (IOException e) | |
| { | |
| throw new DebugException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, DebugException.REQUEST_FAILED, | |
| "Error launching command", e)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java`
around lines 169 - 170, The throw in LaunchConfigurationDelegate currently wraps
e.getCause() which can be null and loses context; change the DebugException
construction to pass the original IOException (the caught variable, likely named
e) as the nested throwable instead of e.getCause(), i.e. when throwing new
DebugException(new Status(..., "Error launching command", e.getCause())),
replace e.getCause() with e so the original exception and stack trace are
preserved.
Comment on lines
+226
to
+243
| while (!finalMonitor.isCanceled() && !launch.isTerminated()) | ||
| { | ||
| Thread.sleep(200); | ||
| } | ||
|
|
||
| if (!launch.isTerminated() && launch.canTerminate()) | ||
| { | ||
| try | ||
| { | ||
| launch.terminate(); | ||
| } | ||
| catch (Exception ignore) | ||
| { | ||
| } | ||
| } | ||
|
|
||
| mainLaunchThread.interrupt(); | ||
| } |
There was a problem hiding this comment.
Cancel watcher interrupts the launch thread even on normal termination.
The loop exits when launch.isTerminated() becomes true, but then still interrupts mainLaunchThread. That can inject spurious interrupts into normal launch flow.
Suggested fix
- while (!finalMonitor.isCanceled() && !launch.isTerminated())
+ while (!finalMonitor.isCanceled() && !launch.isTerminated())
{
Thread.sleep(200);
}
- if (!launch.isTerminated() && launch.canTerminate())
+ if (finalMonitor.isCanceled() && !launch.isTerminated() && launch.canTerminate())
{
try
{
launch.terminate();
}
catch (Exception ignore)
{
}
- }
-
- mainLaunchThread.interrupt();
+ mainLaunchThread.interrupt();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| while (!finalMonitor.isCanceled() && !launch.isTerminated()) | |
| { | |
| Thread.sleep(200); | |
| } | |
| if (!launch.isTerminated() && launch.canTerminate()) | |
| { | |
| try | |
| { | |
| launch.terminate(); | |
| } | |
| catch (Exception ignore) | |
| { | |
| } | |
| } | |
| mainLaunchThread.interrupt(); | |
| } | |
| while (!finalMonitor.isCanceled() && !launch.isTerminated()) | |
| { | |
| Thread.sleep(200); | |
| } | |
| if (finalMonitor.isCanceled() && !launch.isTerminated() && launch.canTerminate()) | |
| { | |
| try | |
| { | |
| launch.terminate(); | |
| } | |
| catch (Exception ignore) | |
| { | |
| } | |
| mainLaunchThread.interrupt(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java`
around lines 226 - 243, The code unconditionally calls
mainLaunchThread.interrupt() after the wait loop, causing spurious interrupts on
normal termination; change the logic so that mainLaunchThread.interrupt() is
only invoked when the wait ended due to cancellation (finalMonitor.isCanceled())
or when you explicitly forced termination—i.e., wrap the launch.terminate() and
mainLaunchThread.interrupt() calls in the cancellation/forced-termination branch
and do not interrupt when launch.isTerminated() returned true normally; update
the block that uses launch.canTerminate(), launch.terminate(), and
mainLaunchThread.interrupt() so the interrupt is only executed on
cancellation/forced termination and keep the existing exception handling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the change and which issue is fixed.
Fixes # (IEP-XXX)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
Chores
Bug Fixes
New Features