IEP-1265: Debug Process Hangs #1023
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (2)
WalkthroughThe recent modifications involve enhancing the Changes
Poem
Tip Migrating from UI to YAML configuration.Use the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml (1)
Verify the impact of disabling process and console factories.
The
<extension>block related toprocessFactoryandconsoleFactoryhas been commented out, which temporarily disables these debugging functionalities. The search results indicate thatCustomIdfProcessFactoryandIdfProcessConsoleFactoryare referenced in several Java files, suggesting dependencies on these classes. Commenting out these extensions could potentially break functionality related to process creation and console management in the debugging environment.
- Files with references:
bundles/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/process/CustomIdfProcessFactory.javabundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/console/IdfProcessConsoleFactory.javabundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/process/StreamListener.javaFurther verification or testing is recommended to ensure that the debugging capabilities are not adversely affected by these changes.
Analysis chain
Line range hint
155-168:
Verify the impact of disabling process and console factories.The
<extension>block related toprocessFactoryandconsoleFactoryhas been commented out, which temporarily disables these debugging functionalities. Ensure that this change does not adversely affect the debugging capabilities required for your development workflow.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of commenting out the process and console factories. # Test: Search for the usage of `CustomIdfProcessFactory` and `IdfProcessConsoleFactory`. Expect: No critical dependencies. rg --type xml -A 5 $'CustomIdfProcessFactory' rg --type xml -A 5 $'IdfProcessConsoleFactory'Length of output: 1306
Script:
#!/bin/bash # Description: Verify the impact of commenting out the process and console factories in Java files. # Test: Search for the usage of `CustomIdfProcessFactory` and `IdfProcessConsoleFactory` in Java files. Expect: No critical dependencies. rg --type java -A 5 $'CustomIdfProcessFactory' rg --type java -A 5 $'IdfProcessConsoleFactory'Length of output: 6195
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml (2 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java
|
@alirana01 Having the same issue with the PR as well. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml (2 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1 hunks)
Files skipped from review due to trivial changes (1)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/plugin.xml
Additional comments not posted (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1)
154-154: Potential Impact of Commenting Out Process Factory ID SettingCommenting out the line that sets the
DebugPlugin.ATTR_PROCESS_FACTORY_IDcould significantly alter how processes are managed during debugging. This change is likely an attempt to troubleshoot the hanging issue, but it's crucial to verify that this does not introduce other issues in the debugging process.Please ensure to test this change across all supported platforms to confirm that it resolves the hanging issue without adverse effects. Consider re-enabling this line conditionally based on further findings or implementing a more targeted fix if this proves to be too broad.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (2 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1 hunks)
Additional comments not posted (2)
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (1)
28-66: The retry mechanism, logging, and exception handling are well-implemented.The retry mechanism is a good addition to handle transient issues that may cause temporary port unavailability. The maximum number of retries (3) and the delay between retries (200ms) seem reasonable. The method correctly returns
falseif the port is still not available after exhausting all retries.The logging statements provide useful information for debugging and monitoring purposes. The log messages are clear and informative.
The method handles
InterruptedExceptionduring the sleep period by restoring the interrupt status and logging an appropriate message. This ensures that the method behaves correctly when interrupted and doesn't silently swallow the exception.bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1)
150-155: LGTM!The conditional check is a good addition to ensure that the port number is only set when the GDB server is intended to be started. This change prevents unnecessary port assignments and enhances the control flow.
| } | ||
| return true; //Fallback not reachable |
There was a problem hiding this comment.
Remove the unreachable fallback return statement.
The fallback return statement return true; at line 66 is unreachable. This statement serves no purpose and can be removed.
Apply this diff to remove the unreachable fallback return statement:
- return true; //Fallback not reachableCommittable 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.
| return true; //Fallback not reachable |
alirana01
changed the title
|
This didn't work for me. Need to check further. Please find the attached IDE error log. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1)
Line range hint
1-265: Summary of review and areas for further attentionThe changes in this file address the port handling logic as mentioned in the PR objectives. However, there are a few areas that might need further attention:
The PR objectives mention debugging process hangs, but the changes in this file don't directly address this issue. Could you please clarify how these changes are expected to resolve the process hangs?
The PR comments indicate that a user (kolipakakondal) is still experiencing issues. It might be beneficial to review the attached log files ("stack.log" and "ide.log") to see if they provide any insights into persisting problems.
Consider adding more comprehensive error handling and logging around the port assignment process. This could help in diagnosing issues if they persist after these changes.
As the PR checklist indicates that verification across all platforms is pending, ensure that thorough testing is conducted on Windows, Linux, and macOS before merging these changes.
Given the complexity of the debugging process and the reported issues, it might be beneficial to implement more detailed logging throughout the launch process. This could help in identifying the exact point where hangs or other issues occur, especially when they're not easily reproducible across all environments.
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (1)
Line range hint
474-479: LGTM: Logical control of GDB client group visibility.The addition of the line to enable/disable the
gdbClientGroupbased on thefDoStartGdbServerselection is a good implementation. It ensures that the GDB client controls are only accessible when the GDB server is not started from the IDE, preventing potential conflicts.Consider adding a comment explaining the logic behind this behavior for better code readability:
gdbClientGroup.setEnabled(!fDoStartGdbServer.getSelection()); +// Enable GDB client controls only when the GDB server is not started from the IDE if (fDoStartGdbServer.getSelection()) { fTargetIpAddress.setText(DefaultPreferences.REMOTE_IP_ADDRESS_LOCALHOST); }bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (1)
28-29: Rename Variables for ClarityFor better readability and understanding, consider renaming the variables:
attemptstoattemptsMadeorattemptCount.retryCounttomaxRetries.This will make it clearer that
attemptsMaderepresents the number of attempts performed, andmaxRetriesindicates the maximum number of retry attempts allowed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (2 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (3 hunks)
🔇 Additional comments (5)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1)
150-155: Approve changes with suggestions for improvementThe changes align with the PR objectives by introducing a conditional port assignment based on whether the GDB server is to be started. This should help address the port handling issues mentioned.
However, I have a few suggestions to enhance this implementation:
Please add a comment explaining the purpose of the
Configuration.getDoStartGdbServer(config)condition. This will improve code readability and maintainability.The PR objectives mention a retry mechanism, but it's not visible in this code segment. Could you please verify if the retry logic is implemented in the
PortChecker.getAvailablePort()method? If not, consider adding a retry mechanism here.As these changes might affect the behavior when the GDB server is not started from the IDE, please ensure that this scenario is thoroughly tested.
To verify the implementation of the retry mechanism and the
getDoStartGdbServermethod, please run the following script:✅ Verification successful
[/run_scripts]
[/run_scripts]
Approve changes with confirmations
The changes align with the PR objectives by introducing a conditional port assignment based on whether the GDB server is to be started. The
PortChecker.getAvailablePortmethod includes a retry mechanism, which addresses the port handling issues mentioned.However, I have a suggestion to enhance this implementation:
- Please add a comment explaining the purpose of the
Configuration.getDoStartGdbServer(config)condition. This will improve code readability and maintainability.- As these changes might affect the behavior when the GDB server is not started from the IDE, please ensure that this scenario is thoroughly tested.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of PortChecker and Configuration classes # Test: Check PortChecker implementation echo "Checking PortChecker implementation:" rg --type java -A 10 "class PortChecker" # Test: Check Configuration.getDoStartGdbServer implementation echo "Checking Configuration.getDoStartGdbServer implementation:" rg --type java -A 5 "getDoStartGdbServer"Length of output: 6944
Script:
#!/bin/bash # Description: Retrieve the implementation of getAvailablePort in PortChecker class # Search for the getAvailablePort method in PortChecker.java ast-grep --lang java --pattern 'public static int getAvailablePort($_)' bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.javaLength of output: 498
Script:
#!/bin/bash # Description: Locate the getAvailablePort method implementation in PortChecker.java using ripgrep # Search for the getAvailablePort method definition rg --type java -C 5 'public\s+static\s+int\s+getAvailablePort\s*\(' bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.javaLength of output: 516
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (3)
112-112: LGTM: Good UI organization.The addition of the
gdbClientGroupvariable is a positive change. It suggests an improvement in the organization of the GDB client controls, which can enhance the user interface's clarity and maintainability.
577-583: LGTM: Improved UI structure for GDB client controls.The changes in the
createGdbClientControlsmethod effectively implement the grouping of GDB client controls. This reorganization enhances the UI structure and improves the visual separation of different control sets. The implementation follows good practices for creating SWT UI components.
Line range hint
1-1337: Overall improvements in GDB client UI organization and control.The changes made to this file focus on enhancing the organization and control of GDB client-related UI elements. The introduction of the
gdbClientGroupand its associated logic improves the structure of the user interface and provides better control over when GDB client options are accessible. These modifications are consistent with the existing code style and improve the overall user experience without introducing any apparent issues.bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (1)
66-66: Remove the Unreachable Fallback Return StatementThe return statement at line 66 is unreachable because all paths return a value within the while loop. This fallback statement is unnecessary and can be safely removed to clean up the code.
| int attempts = 0; | ||
| int retryCount = 3; | ||
| long retryDelayMillis = 200; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Making Retry Parameters Configurable
The retryCount and retryDelayMillis variables are currently hard-coded. To enhance flexibility and adaptability across different environments, consider making these parameters configurable, either through method arguments or configuration files.
| while (attempts <= retryCount) | ||
| { | ||
| return true; | ||
| try (Socket ignored = new Socket("localhost", port)) //$NON-NLS-1$ | ||
| { | ||
| // If the socket opens, the port is in use | ||
| return false; | ||
| } | ||
| catch (IOException e) | ||
| { | ||
| // Port is unavailable, retrying if there are attempts left | ||
| if (attempts == retryCount) | ||
| { | ||
| // After exhausting all retries, return false | ||
| Logger.log("Port " + port + " is not available after " + retryCount + " retries."); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | ||
| return false; // Failure, port is still not available | ||
| } | ||
|
|
||
| attempts++; | ||
|
|
||
| // Log retry attempt | ||
| Logger.log("Attempt " + attempts + " failed, retrying in " + retryDelayMillis + " ms..."); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | ||
|
|
||
| try | ||
| { | ||
| Thread.sleep(retryDelayMillis); | ||
| } | ||
| catch (InterruptedException interruptedException) | ||
| { | ||
| Thread.currentThread().interrupt(); // Restore interrupt status | ||
| Logger.log("Port availability check interrupted."); //$NON-NLS-1$ | ||
| return false; // If interrupted, assume port unavailable and stop | ||
| } | ||
| } | ||
| } | ||
| return true; //Fallback not reachable |
There was a problem hiding this comment.
Fix Logical Error in Port Availability Check
The isPortAvailable method currently has a logical error. It assumes that if it cannot establish a connection to the port after all retries, the port is unavailable. However, inability to connect to a port typically means the port is free and available for use. This method should return true when the port is available (i.e., no service is listening on it) and false when it's in use.
Apply this diff to correct the logic:
while (attempts <= retryCount)
{
try (Socket ignored = new Socket("localhost", port))
{
// If the socket opens, the port is in use
return false;
}
catch (IOException e)
{
// Port is available if we cannot connect
if (attempts == retryCount)
{
- // After exhausting all retries, return false
- Logger.log("Port " + port + " is not available after " + retryCount + " retries.");
- return false; // Failure, port is still not available
+ // After exhausting all retries, return true
+ Logger.log("Port " + port + " is available after " + retryCount + " retries.");
+ return true; // Success, port is available
}
attempts++;
// Log retry attempt
Logger.log("Attempt " + attempts + " failed, retrying in " + retryDelayMillis + " ms...");
try
{
Thread.sleep(retryDelayMillis);
}
catch (InterruptedException interruptedException)
{
Thread.currentThread().interrupt(); // Restore interrupt status
Logger.log("Port availability check interrupted.");
return false; // If interrupted, assume port unavailable and stop
}
}
}
- return true; //Fallback not reachable
+ return true; // Port is available after retries📝 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.
| while (attempts <= retryCount) | |
| { | |
| return true; | |
| try (Socket ignored = new Socket("localhost", port)) //$NON-NLS-1$ | |
| { | |
| // If the socket opens, the port is in use | |
| return false; | |
| } | |
| catch (IOException e) | |
| { | |
| // Port is unavailable, retrying if there are attempts left | |
| if (attempts == retryCount) | |
| { | |
| // After exhausting all retries, return false | |
| Logger.log("Port " + port + " is not available after " + retryCount + " retries."); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | |
| return false; // Failure, port is still not available | |
| } | |
| attempts++; | |
| // Log retry attempt | |
| Logger.log("Attempt " + attempts + " failed, retrying in " + retryDelayMillis + " ms..."); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | |
| try | |
| { | |
| Thread.sleep(retryDelayMillis); | |
| } | |
| catch (InterruptedException interruptedException) | |
| { | |
| Thread.currentThread().interrupt(); // Restore interrupt status | |
| Logger.log("Port availability check interrupted."); //$NON-NLS-1$ | |
| return false; // If interrupted, assume port unavailable and stop | |
| } | |
| } | |
| } | |
| return true; //Fallback not reachable | |
| while (attempts <= retryCount) | |
| { | |
| try (Socket ignored = new Socket("localhost", port)) //$NON-NLS-1$ | |
| { | |
| // If the socket opens, the port is in use | |
| return false; | |
| } | |
| catch (IOException e) | |
| { | |
| // Port is available if we cannot connect | |
| if (attempts == retryCount) | |
| { | |
| // After exhausting all retries, return true | |
| Logger.log("Port " + port + " is available after " + retryCount + " retries."); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | |
| return true; // Success, port is available | |
| } | |
| attempts++; | |
| // Log retry attempt | |
| Logger.log("Attempt " + attempts + " failed, retrying in " + retryDelayMillis + " ms..."); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ | |
| try | |
| { | |
| Thread.sleep(retryDelayMillis); | |
| } | |
| catch (InterruptedException interruptedException) | |
| { | |
| Thread.currentThread().interrupt(); // Restore interrupt status | |
| Logger.log("Port availability check interrupted."); //$NON-NLS-1$ | |
| return false; // If interrupted, assume port unavailable and stop | |
| } | |
| } | |
| } | |
| return true; // Port is available after retries |
|
Hi @alirana01 , thanks for the PR. I verified the debugging with the ESP32-Wrover-Kit, and it seems to be working fine. I no longer see the issue I reported earlier. However, I previously tested with the ESP32-C6, but my cable is currently missing, so I cannot verify it at this moment wit the c6. |
|
Hi @AndriiFilippov Please test further |
|
@alirana01 hi ! Tested under:
and start debug with GDB I am not able to stop the process.
but it doesn't.
If those ports are occupied:
then it just throws error message, not looking for available port: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1)
150-153: Approve the condition, but suggest improvementsThe addition of the condition to set the port number when the GDB server is started is a good step towards addressing the port handling logic issues. However, there are a few points to consider:
The current implementation doesn't include a retry mechanism or port verification as mentioned in the PR summary. Consider implementing these to make the solution more robust.
This change doesn't address the concerns raised by AndriiFilippov about endless retries and not switching to available ports. Consider adding logic to handle occupied ports and switch to available ones.
It would be helpful to add a comment explaining the rationale behind this change, especially in relation to the issues it's meant to solve.
Consider expanding this implementation to include:
- A retry mechanism with a maximum number of attempts
- Port verification to ensure the chosen port is actually available
- Logic to switch to an available port if the default is occupied
Example:
if (Configuration.getDoStartGdbServer(config)) { int port = DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT; int maxAttempts = 5; for (int attempt = 0; attempt < maxAttempts; attempt++) { if (PortChecker.isPortAvailable(port)) { config.setAttribute(IGDBJtagConstants.ATTR_PORT_NUMBER, port); break; } port++; // Try the next port } if (attempt == maxAttempts) { throw new CoreException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, "Unable to find an available port after " + maxAttempts + " attempts")); } }Also, consider adding a comment explaining the purpose of this block:
// Set the GDB server port when starting the server from the IDE. // This ensures we use an available port and avoid conflicts.bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (1)
577-583: LGTM: GDB client group initialization.The initialization and configuration of the
gdbClientGroupare implemented correctly. The use of localized strings for the group title is good for internationalization.For consistency with other parts of the code, consider extracting the string key to a constant:
- gdbClientGroup.setText(Messages.getString("DebuggerTab.gdbSetupGroup_Text")); //$NON-NLS-1$ + gdbClientGroup.setText(Messages.getString(Messages.DebuggerTab_gdbSetupGroup_Text));And define the constant in the
Messagesclass:public static final String DebuggerTab_gdbSetupGroup_Text = "DebuggerTab.gdbSetupGroup_Text";This approach would make it easier to manage and update string keys throughout the codebase.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (4)
31-31: Question the Necessity ofsetReuseAddress(true)Calling
serverSocket.setReuseAddress(true);after the socket has already been bound may not have any effect. Since the primary goal is to check if the port is available, and not to reuse the address, this line may be unnecessary. Removing it can simplify the code without impacting functionality.Apply this diff to remove the unnecessary line:
Line range hint
43-47: Improve Handling When No Available Port Is FoundWhen the maximum number of attempts is reached without finding an available port, the method currently returns the last checked port, which may still be unavailable. This could lead to downstream failures. Consider throwing an exception or returning a sentinel value (e.g.,
-1) to clearly indicate that no available port was found.Option 1: Throw an Exception
Modify the method to throw an
IOExceptionwhen no port is available:if (attemptsCount >= MAX_ATTEMPS) { Logger.log(Messages.PortChecker_AttemptLimitExceededMsg); - return port; + throw new IOException("No available ports found after " + MAX_ATTEMPS + " attempts."); }Option 2: Return a Sentinel Value
Return
-1to indicate failure:if (attemptsCount >= MAX_ATTEMPS) { Logger.log(Messages.PortChecker_AttemptLimitExceededMsg); - return port; + return -1; }Ensure that any calling methods handle this new behavior appropriately.
7-8: Remove Unused Importjava.io.IOExceptionThe import statement
import java.io.IOException;is added but not used in the code. Removing unused imports can improve code readability and reduce clutter.Apply this diff to remove the unused import:
29-32: Consider Binding to a Specific Local AddressIf the application runs on a machine with multiple network interfaces, you might want to specify the local address to bind the
ServerSocketto. This can prevent binding to unintended network interfaces.Modify the
ServerSocketinstantiation to specify the local address if necessary:try (ServerSocket serverSocket = new ServerSocket(port, backlog, InetAddress.getByName("localhost")))Replace
"localhost"with the appropriate local address if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (2 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java (1 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1)
Line range hint
1-253: Summary and Next StepsThe changes made to the
Launch.javafile partially address the port handling issues mentioned in the PR objectives. However, to fully resolve the problems and address the concerns raised in the comments, consider the following next steps:
- Implement a more robust port selection mechanism with retries and verification.
- Address the concerns about endless retries and switching to available ports.
- Ensure the solution works across all platforms (Windows, Linux, macOS) as mentioned in the PR summary.
- Add comprehensive error handling and logging to aid in debugging.
- Update the PR description to accurately reflect the implemented changes and any remaining tasks.
To improve the overall architecture and maintainability of the debugging process:
- Consider extracting the port selection logic into a separate utility class. This would make it easier to test and reuse across different parts of the codebase.
- Implement a configuration option to allow users to specify a range of acceptable ports. This would provide more flexibility and potentially avoid conflicts with other applications.
- Add telemetry (if not already present) to track the frequency and nature of port-related issues. This data could inform future improvements to the debugging process.
To ensure the changes work as expected across different scenarios, consider adding the following verification steps:
These verification steps will help ensure that the changes are consistent with the rest of the codebase and that no conflicting implementations exist.
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java (1)
96-97:⚠️ Potential issueConsider maintaining consistency in port configuration retrieval
The change simplifies the GDB server port retrieval by using a default value directly. However, this approach differs from how other ports (Telnet and TCL) are handled in the same method, which still use configuration attributes.
Potential issues:
- Loss of flexibility: Users can no longer configure custom GDB server ports through launch configurations.
- Inconsistency: The GDB server port is handled differently from Telnet and TCL ports.
- Backward compatibility: Existing setups relying on custom GDB server port configurations may break.
Consider the following alternatives:
- Revert to using configuration attributes for consistency:
int port = PortChecker.getAvailablePort(configuration.getAttribute(ConfigurationAttributes.GDB_SERVER_GDB_PORT_NUMBER, DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT));
- Apply the same simplification to Telnet and TCL ports for consistency.
- Add a comment explaining why the GDB server port is handled differently, if there's a specific reason for this change.
To ensure this change doesn't break existing configurations, please run the following script:
This will help identify any existing launch configurations that might be affected by this change.
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (2)
112-114: LGTM: New group for GDB client controls.The addition of the
gdbClientGroupvariable improves the organization of the UI elements related to the GDB client settings.
Line range hint
474-481: LGTM: Improved control flow for GDB server/client interaction.This change enhances the user experience by:
- Disabling the GDB client group when the GDB server is set to start locally, preventing conflicting configurations.
- Automatically setting the IP address to "localhost" when starting the GDB server locally, which is the correct behavior.
These modifications ensure a more intuitive and error-resistant UI.
bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (1)
29-37:⚠️ Potential issueHandle InterruptedException in Port Checking Loop
In the
getAvailablePortmethod, if anInterruptedExceptionoccurs duringThread.sleep(), the interrupt status is not restored or handled, which might lead to unexpected behavior. Ensure that the interrupt status is preserved or appropriately handled.If the sleep is interrupted, consider restoring the interrupt status:
// Example code if sleep is used within the loop try { Thread.sleep(retryDelayMillis); } catch (InterruptedException interruptedException) { Thread.currentThread().interrupt(); // Restore interrupt status Logger.log("Port checking was interrupted."); throw new IOException("Port checking was interrupted.", interruptedException); }Adjust the code according to your implementation needs.
Likely invalid or redundant comment.
| try (ServerSocket serverSocket = new ServerSocket(port)) | ||
| { | ||
| return false; | ||
| serverSocket.setReuseAddress(true); | ||
| return true; | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| return true; | ||
| Logger.log("Port: " + port + " is not available"); //$NON-NLS-1$ //$NON-NLS-2$ | ||
| return false; |
There was a problem hiding this comment.
Catch Specific Exceptions Instead of Generic Exception
Catching the generic Exception can mask other unintended exceptions and make debugging harder. It's better to catch the more specific IOException, which is the expected exception type for socket operations.
Apply this diff to catch IOException specifically:
-} catch (Exception e) {
+} catch (IOException e) {
Logger.log("Port: " + port + " is not available"); //$NON-NLS-1$ //$NON-NLS-2$
return false;
}📝 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.
| try (ServerSocket serverSocket = new ServerSocket(port)) | |
| { | |
| return false; | |
| serverSocket.setReuseAddress(true); | |
| return true; | |
| } | |
| catch (Exception e) | |
| { | |
| return true; | |
| Logger.log("Port: " + port + " is not available"); //$NON-NLS-1$ //$NON-NLS-2$ | |
| return false; | |
| try (ServerSocket serverSocket = new ServerSocket(port)) | |
| { | |
| serverSocket.setReuseAddress(true); | |
| return true; | |
| } | |
| catch (IOException e) | |
| { | |
| Logger.log("Port: " + port + " is not available"); //$NON-NLS-1$ //$NON-NLS-2$ | |
| return false; |
|
@alirana01 hi ! Issue still persist |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1)
150-153: Approve with suggestion: Consider adding a fallback mechanism for occupied ports.The change simplifies port assignment and aligns with the PR objective of addressing process hang issues. However, to enhance robustness, consider implementing a fallback mechanism for scenarios where the default port is occupied.
Here's a suggested improvement:
if (Configuration.getDoStartGdbServer(config)) { - config.setAttribute(IGDBJtagConstants.ATTR_PORT_NUMBER, DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT); + int port = DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT; + while (!PortChecker.isPortAvailable(port) && port < DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT + 10) { + port++; + } + if (PortChecker.isPortAvailable(port)) { + config.setAttribute(IGDBJtagConstants.ATTR_PORT_NUMBER, port); + } else { + throw new CoreException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, "No available ports found for GDB server")); + } }This suggestion adds a retry mechanism to find an available port within a range, addressing the concern raised by user AndriiFilippov about endless retries without switching to an available port.
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (1)
474-474: Improved control flow for GDB client group.The addition of this line enhances the user experience by disabling the GDB client controls when the GDB server is set to start. This prevents potential conflicts in the debugging configuration.
Consider adding a comment explaining the reason for this behavior:
gdbClientGroup.setEnabled(!fDoStartGdbServer.getSelection()); +// Disable GDB client controls when the server is set to start to prevent configuration conflictsbundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (2)
31-31: Redundant call tosetReuseAddress(true)Setting
setReuseAddress(true)after theServerSocketis bound may not have the intended effect, as the socket is already bound by that point. Since the socket is used only for a quick availability check and is immediately closed, this call might be unnecessary.Apply this diff to remove the redundant call:
try (ServerSocket serverSocket = new ServerSocket(port)) { - serverSocket.setReuseAddress(true); return true; }
Line range hint
43-59: Handle the case when no available port is found after maximum attemptsCurrently, if no available port is found after the maximum number of attempts,
getAvailablePortreturns the last attempted port number, which may not be available. This could lead to issues downstream when attempting to use an unavailable port. Consider throwing an exception to indicate that a free port was not found.Apply this diff to throw an exception when no port is available:
+public static int getAvailablePort(int port) throws IOException { int attemptsCount = 0; while (!isPortAvailable(port) && attemptsCount < MAX_ATTEMPS) { Logger.log(String.format(Messages.PortChecker_PortNotAvailable, port, port + 1)); port += 1; attemptsCount += 1; } if (attemptsCount >= MAX_ATTEMPS) { Logger.log(Messages.PortChecker_AttemptLimitExceededMsg); - return port; + throw new IOException("No available port found after " + MAX_ATTEMPS + " attempts."); } Logger.log(String.format(Messages.PortChecker_PortIsAvailable, port)); return port; }Ensure that any code calling this method handles the
IOExceptionappropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (2 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/Configuration.java (1 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (1 hunks)
- bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (3 hunks)
🧰 Additional context used
🔇 Additional comments (3)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/ui/TabDebugger.java (2)
112-112: New group variable for GDB client controls.A new
GroupvariablegdbClientGrouphas been added to the class. This is likely used to group and manage GDB client-related UI controls.
577-583: Refactored GDB client controls into a class member group.The
createGdbClientControlsmethod now initializes the class membergdbClientGroupinstead of a localGroupvariable. This change allows for better management and access to the GDB client controls throughout the class.bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java (1)
29-37: Proper implementation of port availability checkThe updated
isPortAvailablemethod usingServerSocketappropriately checks if a port can be bound, which is a reliable way to determine port availability.
| .getAvailablePort(DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT); | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Dynamic port selection improved, but Telnet port requires attention
The implementation of PortChecker.getAvailablePort with a retry limit enhances the robustness of dynamic port selection for the GDB and TCL ports, effectively addressing potential port conflicts and preventing endless retries.
However, the telnet_port still does not utilize PortChecker.getAvailablePort. To ensure consistency and prevent similar issues with the Telnet port, please apply the dynamic port selection logic with retry limits to the telnet_port as well.
- Apply
PortChecker.getAvailablePortwith retry limits to the Telnet port configuration.
🔗 Analysis chain
Approve the dynamic port selection, but suggest improvements
The introduction of PortChecker.getAvailablePort() for the GDB port is a good step towards resolving port conflicts and addressing the debugging process hangs. However, there are a few points to consider:
-
This change is only applied to the GDB port. For consistency and to fully address potential conflicts, consider applying the same logic to the Telnet and TCL ports as well.
-
To prevent the endless retry issue reported by AndriiFilippov, consider adding a retry limit or a timeout mechanism to the port selection process.
-
Verify if this change resolves the reported debugging hangs across different platforms (Windows, Linux, macOS) and with various ESP-IDF versions.
Here's a suggested implementation for applying the dynamic port selection to all ports:
private static int getAvailablePort(int defaultPort, int maxRetries) {
for (int i = 0; i < maxRetries; i++) {
int port = PortChecker.getAvailablePort(defaultPort);
if (port != defaultPort) {
return port;
}
}
throw new RuntimeException("Unable to find an available port after " + maxRetries + " attempts");
}
// In getGdbServerCommandLineArray method:
int gdbPort = getAvailablePort(DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT, 10);
lst.add("-c");
lst.add("gdb_port " + gdbPort);
int telnetPort = getAvailablePort(DefaultPreferences.GDB_SERVER_TELNET_PORT_NUMBER_DEFAULT, 10);
lst.add("-c");
lst.add("telnet_port " + telnetPort);
int tclPort = getAvailablePort(DefaultPreferences.GDB_SERVER_TCL_PORT_NUMBER_DEFAULT, 10);
lst.add("-c");
lst.add("tcl_port " + tclPort);This implementation applies the dynamic port selection to all ports and includes a retry limit to prevent endless loops.
To ensure this change resolves the reported issues, please run the following verification steps:
Please run these tests and provide the results to ensure the changes work as expected across different environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the changes across different platforms and ESP-IDF versions
# Test on different platforms
platforms=("windows" "linux" "macos")
for platform in "${platforms[@]}"; do
echo "Testing on $platform"
# Add platform-specific test commands here
done
# Test with different ESP-IDF versions
versions=("4.4" "5.0" "5.1")
for version in "${versions[@]}"; do
echo "Testing with ESP-IDF v$version"
# Add version-specific test commands here
done
# Verify port selection logic
echo "Verifying port selection logic"
rg "PortChecker.getAvailablePort" -A 10
Length of output: 2411
Script:
#!/bin/bash
# Verify dynamic port selection for telnet_port
echo "Searching for PortChecker.getAvailablePort usage in telnet_port"
rg "telnet_port" -A 10 | rg "PortChecker.getAvailablePort"
# Check implementation of getAvailablePort for retry limits
echo "Checking implementation of getAvailablePort for retry limits"
rg "getAvailablePort" -A 10 bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/PortChecker.java
Length of output: 761
Script:
#!/bin/bash
# Locate the PortChecker.java file in the repository
echo "Searching for PortChecker.java in the repository"
fd "PortChecker.java" . -type f
# If the file is found, check for the implementation of getAvailablePort with retry limits
# Replace <path_to_PortChecker.java> with the actual path from the previous command's output
# Example:
# rg "def getAvailablePort" <path_to_PortChecker.java> -A 10
# Verify if PortChecker.getAvailablePort is used for telnet_port
echo "Searching for PortChecker.getAvailablePort usage in telnet_port"
rg "telnet_port" -A 10 | rg "PortChecker.getAvailablePort"
Length of output: 654
Script:
#!/bin/bash
# Corrected script to locate PortChecker.java and verify its implementation
# Locate the PortChecker.java file in the repository
echo "Searching for PortChecker.java in the repository"
fd "PortChecker.java" . --type f
# If the file is found, check for the implementation of getAvailablePort with retry limits
# Replace <path_to_PortChecker.java> with the actual path from the previous command's output
# Example:
# rg "getAvailablePort" <path_to_PortChecker.java> -A 10
# Assuming the file was found at bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/PortChecker.java
echo "Inspecting getAvailablePort method for retry limits"
rg "getAvailablePort" -A 10 bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/PortChecker.java
# Verify if PortChecker.getAvailablePort is used for telnet_port
echo "Searching for PortChecker.getAvailablePort usage in telnet_port"
rg "telnet_port" -A 10 | rg "PortChecker.getAvailablePort"
Length of output: 965
Script:
#!/bin/bash
# Corrected script to locate PortChecker.java and verify its implementation
# Locate the PortChecker.java file in the repository
echo "Searching for PortChecker.java in the repository"
fd "PortChecker.java" . --type file
# Check implementation of getAvailablePort for retry limits in the correct PortChecker.java
echo "Inspecting getAvailablePort method for retry limits"
rg "getAvailablePort" -A 10 bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/PortChecker.java
# Verify if PortChecker.getAvailablePort is used for telnet_port
echo "Searching for PortChecker.getAvailablePort usage in telnet_port"
rg "telnet_port" -A 10 | rg "PortChecker.getAvailablePort"
Length of output: 1095
|
@AndriiFilippov please use these guidelines there must be another way to occupy the ports to verify this anyway at times the serial id for the connected boards is similar we need to specify it different for each board I have attached a conversation on the original jira ticket for you to track and verify. |
|
@alirana01 hi! Tested, but it seems the issue is still persist. Reproduce:
|
…er processes" This reverts commit a926bd6.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java (2)
701-713: Consider enhancing the cleanup implementation.The cleanup implementation could be more robust:
- Add debug logging to track cleanup actions
- Consider handling cleanup of other resources beyond just terminating the launch
- Add error handling for the terminate() call
protected void cleanupLaunch(ILaunch launch) throws DebugException { + if (Activator.getInstance().isDebugging()) { + System.out.println("openocd.LaunchConfigurationDelegate.cleanupLaunch()"); + } if (launch instanceof GdbLaunch) { - launch.terminate(); + try { + launch.terminate(); + } catch (DebugException e) { + Activator.log(e); + throw e; + } } }
Line range hint
714-734: Improve error handling in checkBinaryDetails method.The current implementation has empty catch blocks and inconsistent error handling:
- Empty catch blocks silently ignore exceptions
- The doStartServer variable initialization is redundant
protected IPath checkBinaryDetails(final ILaunchConfiguration config) throws CoreException { - boolean doStartServer = true; try { - doStartServer = Configuration.getDoStartGdbServer(config); + boolean doStartServer = Configuration.getDoStartGdbServer(config); + if (doStartServer) + { + String configOptions = Configuration.getGdbServerOtherConfig(config); + if (configOptions.isEmpty()) + { + throw new CoreException( + new Status(IStatus.ERROR, Activator.PLUGIN_ID, + "Missing mandatory configuration. Fill-in the 'Config options:' field in the Debugger tab.")); + } + } } catch (CoreException e) { - ; + Activator.log(e); + throw e; } - if (doStartServer) - { - String configOptions = ""; - try - { - configOptions = Configuration.getGdbServerOtherConfig(config); - } - catch (CoreException e) - { - ; - } - - if (configOptions.isEmpty()) - { - throw new CoreException( - new Status(IStatus.ERROR, Activator.PLUGIN_ID, "Missing mandatory configuration. " - + "Fill-in the 'Config options:' field in the Debugger tab.")); - } - } IPath path = super.checkBinaryDetails(config); return path; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java(1 hunks)
|
|
||
| @Override | ||
| /** | ||
| * This method takes care of cleaning up any resources allocated by the launch, as early as | ||
| * the call to getLaunch(), whenever the launch is cancelled or does not complete properly. | ||
| * @since 5.0 */ | ||
| protected void cleanupLaunch(ILaunch launch) throws DebugException | ||
| { | ||
| if (launch instanceof GdbLaunch) | ||
| { | ||
| launch.terminate(); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Potential race condition in cleanupLaunch method.
The cleanupLaunch method lacks synchronization, which may lead to race conditions when multiple debug sessions run concurrently.
🔗 Analysis chain
Verify cleanup behavior during concurrent debug sessions.
The cleanup implementation should be tested for race conditions when multiple debug sessions are running simultaneously.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential race conditions in cleanup handling
rg -A 5 "terminate|cleanup" --type java
Length of output: 21202
Script:
#!/bin/bash
# Search for synchronization mechanisms in LaunchConfigurationDelegate.java related to cleanup
fd "LaunchConfigurationDelegate.java" | xargs ast-grep --pattern '
class LaunchConfigurationDelegate {
$$$
cleanupLaunch($_) {
$$$
synchronized($_) {
$$$
}
}
}'
Length of output: 184
|
Hi @alirana01 Tested environment: Test Procedure: 
I then clicked on the Terminate button, but it failed to terminate the debug session, causing the IDE to hang. I was unable to perform any actions, so I had to restart it. After restarting, I clicked on Debug again. This time, it didn’t initiate debug mode and got stuck at the "Configuring GDB" state, as shown below. Nothing is happening even when I click on terminate button 
When I verify in terminal, I see that port 3333 occupied I restarted Eclipse, increased the flash size, rebuilt the project, and flashed it again. When I clicked on Debug this time, no flash mapping issues were reported. However, it didn’t stop at the breakpoint. I decided to kill and restart the debug session, but then I encountered the same old issue, as shown below. 
|
|
After further troubleshooting, I was able to debug it properly with esp32c6. However, when issues such as connecting to the board, timeout errors, auto_probe error, or flash mapping problems occur, the IDE does not handle them gracefully and encounters the previously reported issues, such as the IDE getting stuck.
|
|
The same problem occurs if the 3333 port is occupied (e.g., when I start OpenOCD from the terminal to block the port) and then try to start the debug session from Eclipse (both server and client).
|
|
@kolipakakondal I have updated the code. I had to use some internal classes from eclipse as seemingly they seem to be fine but required a lot of changes and for some reason they didn't bother making the internal implementation's constructor extendable so reused their code and did the required modifications so killing a process would not bounce back errors. This should resolve some of the issues you mentioned I couldn't get to reproduce the issue for debugger not opening up the files except once. Please try to test if this resolves it better @AndriiFilippov also take a look and try with new builds |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (2)
177-178: Consider enhancing process initialization error handlingWhile storing process references is good, the error handling could be more robust. Consider adding validation and proper cleanup if process initialization fails.
openOcdServerProcess = addServerProcess(Configuration.getGdbServerCommandName(fConfig)); +if (openOcdServerProcess == null) { + throw new CoreException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, + "Failed to initialize OpenOCD server process")); +} monitor.worked(1);Also applies to: 192-193
270-274: Make getProcesses more null-safeThe current implementation could return null elements in the array. Consider making it more robust.
@Override public IProcess[] getProcesses() { - return new IProcess[] { openOcdServerProcess, gdbIProcess }; + return Stream.of(openOcdServerProcess, gdbIProcess) + .filter(Objects::nonNull) + .toArray(IProcess[]::new); }bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java (1)
Line range hint
701-735: Improve error handling and validation incheckBinaryDetails.The current implementation has several issues that should be addressed:
- Empty catch blocks silently swallow exceptions, making it difficult to diagnose issues.
- The validation logic could be more robust by combining both configuration checks.
Consider applying this refactor:
@Override protected IPath checkBinaryDetails(final ILaunchConfiguration config) throws CoreException { - boolean doStartServer = true; - try { - doStartServer = Configuration.getDoStartGdbServer(config); - } catch (CoreException e) { - ; - } - - if (doStartServer) { - // If we should start the server, there must be a configuration - // present, otherwise refuse to start. - String configOptions = ""; - try { - configOptions = Configuration.getGdbServerOtherConfig(config); - } catch (CoreException e) { - ; - } - - if (configOptions.isEmpty()) { - throw new CoreException( - new Status(IStatus.ERROR, Activator.PLUGIN_ID, "Missing mandatory configuration. " - + "Fill-in the 'Config options:' field in the Debugger tab.")); //$NON-NLS-1$ - } - } + boolean doStartServer = Configuration.getDoStartGdbServer(config); + if (doStartServer) { + String configOptions = Configuration.getGdbServerOtherConfig(config); + if (configOptions == null || configOptions.trim().isEmpty()) { + throw new CoreException( + new Status(IStatus.ERROR, Activator.PLUGIN_ID, + "Server configuration is missing. Please provide the required configuration options in the 'Config options:' field under the Debugger tab.")); //$NON-NLS-1$ + } + } IPath path = super.checkBinaryDetails(config); return path; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/GdbBackend.java(2 hunks)bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java(6 hunks)bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java(1 hunks)bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/process/IdfRuntimeProcess.java(2 hunks)
🔇 Additional comments (6)
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/process/IdfRuntimeProcess.java (2)
19-19: LGTM: Import addition is appropriate
The StringUtil import is correctly placed and necessary for the new string validation logic.
42-45: Consider making GDB process identification more robust
While the current implementation works, relying on the process label containing "gdb" might be fragile. Consider:
- Using a more explicit identifier or constant for GDB process identification
- Adding documentation explaining why GDB processes need different stream handling
Let's verify the process label handling across the codebase:
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/Launch.java (2)
61-62: LGTM: Good addition of process tracking fields
The addition of these fields improves process lifecycle management by maintaining references to both the OpenOCD server and GDB client processes.
153-156: Verify port handling across different platforms
Based on PR feedback, there are still issues with port conflicts. Let's verify the port handling behavior:
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/LaunchConfigurationDelegate.java (1)
Line range hint 701-735: Verify thread safety in cleanup operations.
The cleanupLaunch method is called from multiple error handling paths, which could lead to race conditions if invoked concurrently.
Let's verify the thread safety of cleanup operations:
bundles/com.espressif.idf.debug.gdbjtag.openocd/src/com/espressif/idf/debug/gdbjtag/openocd/dsf/GdbBackend.java (1)
936-960: Review the condition for process termination
In the MonitorJob class, the condition if(fProcess.isAlive() && Activator.getInstance().isDebugging()) checks if the process is alive and debugging is enabled before calling destroy() and updating the backend state. This may prevent proper cleanup when debugging is not active.
Please verify if the condition should exclude the Activator.getInstance().isDebugging() check to ensure that destroy() is called whenever the process is alive, regardless of the debugging state.
Consider modifying the condition as follows:
-if(fProcess.isAlive() && Activator.getInstance().isDebugging())
+if(fProcess.isAlive())| if (Configuration.getDoStartGdbServer(config)) | ||
| { | ||
| config.setAttribute(IGDBJtagConstants.ATTR_PORT_NUMBER, DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT); | ||
| } |
There was a problem hiding this comment.
Port handling logic needs improvement
The current implementation sets a fixed default port when the GDB server starts, but according to PR feedback, this doesn't handle port conflicts effectively. When the default port (3333) is occupied, the system should retry with alternative ports.
Consider implementing a more robust port selection mechanism:
if (Configuration.getDoStartGdbServer(config))
{
- config.setAttribute(IGDBJtagConstants.ATTR_PORT_NUMBER, DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT);
+ int port = PortChecker.findAvailablePort(DefaultPreferences.GDB_SERVER_GDB_PORT_NUMBER_DEFAULT);
+ if (port == -1) {
+ throw new CoreException(new Status(IStatus.ERROR, Activator.PLUGIN_ID,
+ "Unable to find available port for GDB server"));
+ }
+ config.setAttribute(IGDBJtagConstants.ATTR_PORT_NUMBER, port);
}Committable suggestion skipped: line range outside the PR's diff.
| @Override | ||
| public void terminate() throws DebugException | ||
| { | ||
| super.terminate(); | ||
| for(IProcess process : getProcesses()) | ||
| { | ||
| if (process != null) | ||
| { | ||
| try | ||
| { | ||
| process.terminate(); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| e.printStackTrace(); | ||
| } | ||
|
|
||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in terminate method
While the error handling is present, it could be enhanced:
- Use proper logging instead of printStackTrace
- Consider aggregating multiple termination errors
@Override
public void terminate() throws DebugException {
super.terminate();
+ List<Exception> errors = new ArrayList<>();
for(IProcess process : getProcesses()) {
if (process != null) {
try {
process.terminate();
} catch (Exception e) {
- e.printStackTrace();
+ Activator.log(Status.ERROR, "Failed to terminate process: " + process.getLabel(), e);
+ errors.add(e);
}
}
}
+ if (!errors.isEmpty()) {
+ throw new DebugException(new Status(IStatus.ERROR, Activator.PLUGIN_ID,
+ "Failed to terminate one or more processes", errors.get(0)));
+ }
}📝 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.
| @Override | |
| public void terminate() throws DebugException | |
| { | |
| super.terminate(); | |
| for(IProcess process : getProcesses()) | |
| { | |
| if (process != null) | |
| { | |
| try | |
| { | |
| process.terminate(); | |
| } | |
| catch (Exception e) | |
| { | |
| e.printStackTrace(); | |
| } | |
| } | |
| } | |
| } | |
| @Override | |
| public void terminate() throws DebugException | |
| { | |
| super.terminate(); | |
| List<Exception> errors = new ArrayList<>(); | |
| for(IProcess process : getProcesses()) | |
| { | |
| if (process != null) | |
| { | |
| try | |
| { | |
| process.terminate(); | |
| } | |
| catch (Exception e) | |
| { | |
| Activator.log(Status.ERROR, "Failed to terminate process: " + process.getLabel(), e); | |
| errors.add(e); | |
| } | |
| } | |
| } | |
| if (!errors.isEmpty()) { | |
| throw new DebugException(new Status(IStatus.ERROR, Activator.PLUGIN_ID, | |
| "Failed to terminate one or more processes", errors.get(0))); | |
| } | |
| } |
| protected Process launchGDBProcess() throws CoreException { | ||
| // Keep calling deprecated getGDBCommandLine() in case it was overridden | ||
| String command = getGDBCommandLine(); | ||
| // Keep calling deprecated launchGDBProcess(String) in case it was | ||
| // overridden | ||
| return launchGDBProcess(command); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate usage of deprecated methods
The methods getGDBCommandLine() and launchGDBProcess(String) are deprecated. It's recommended to update these to use getGDBCommandLineArray() and launchGDBProcess(String[]) respectively for better maintainability and to adhere to modern practices.
Apply this diff to refactor the methods:
-protected Process launchGDBProcess() throws CoreException {
- // Keep calling deprecated getGDBCommandLine() in case it was overridden
- String command = getGDBCommandLine();
- // Keep calling deprecated launchGDBProcess(String) in case it was overridden
- return launchGDBProcess(command);
-}
+protected Process launchGDBProcess() throws CoreException {
+ String[] commandArray = getGDBCommandLineArray();
+ return launchGDBProcess(commandArray);
+}📝 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.
| protected Process launchGDBProcess() throws CoreException { | |
| // Keep calling deprecated getGDBCommandLine() in case it was overridden | |
| String command = getGDBCommandLine(); | |
| // Keep calling deprecated launchGDBProcess(String) in case it was | |
| // overridden | |
| return launchGDBProcess(command); | |
| } | |
| protected Process launchGDBProcess() throws CoreException { | |
| String[] commandArray = getGDBCommandLineArray(); | |
| return launchGDBProcess(commandArray); | |
| } |
| inputReader = new BufferedReader(new InputStreamReader(getMIInputStream())); | ||
| String line; | ||
| while ((line = inputReader.readLine()) != null) { | ||
| line = line.trim(); | ||
| if (line.endsWith("(gdb)")) { //$NON-NLS-1$ | ||
| success = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Failed to read initial prompt, check for error | ||
| if (!success) { | ||
| errorReader = new BufferedReader(new InputStreamReader(getMIErrorStream())); | ||
| String errorInfo = errorReader.readLine(); | ||
| if (errorInfo == null) { | ||
| errorInfo = "GDB prompt not read"; //$NON-NLS-1$ | ||
| } | ||
| gdbLaunchRequestMonitor | ||
| .setStatus(new Status(IStatus.ERROR, GdbPlugin.PLUGIN_ID, -1, errorInfo, null)); | ||
| } | ||
| } catch (IOException e) { | ||
| success = false; | ||
| gdbLaunchRequestMonitor.setStatus( | ||
| new Status(IStatus.ERROR, GdbPlugin.PLUGIN_ID, -1, "Error reading GDB output", e)); //$NON-NLS-1$ | ||
| } | ||
|
|
||
| // In the case of failure, close the MI streams so | ||
| // they are not leaked. | ||
| if (!success) { | ||
| if (inputReader != null) { | ||
| try { | ||
| inputReader.close(); | ||
| } catch (IOException e) { | ||
| } | ||
| } | ||
| if (errorReader != null) { | ||
| try { | ||
| errorReader.close(); | ||
| } catch (IOException e) { | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Ensure streams are closed to prevent resource leaks
In the run method of the GDBProcessStep class, the inputReader and errorReader are closed only in the failure case. To prevent potential resource leaks, these streams should also be closed after successfully reading the GDB prompt.
Apply this diff to close the streams in both success and failure scenarios:
...
+ } finally {
+ if (inputReader != null) {
+ try {
+ inputReader.close();
+ } catch (IOException e) {
+ // Handle exception or log it
+ }
+ }
+ if (errorReader != null) {
+ try {
+ errorReader.close();
+ } catch (IOException e) {
+ // Handle exception or log it
+ }
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
|
@alirana01 hi ! I am able to debug, but on debug start I do see this errors. I am continuing to test |
|
@AndriiFilippov @kolipakakondal I have updated with recent changes please take a look again |
|
@alirana01 I am able to Debug but when I stop process sometime (random) it is throws this error and delete Debug config OR sometime it stops process successfully (Activity Monitor shows no openOCD or GDB process) + port 3333 free as well, but it gets stuck on the next execution. without any error or warning and I am not able to terminate process. just openocd process occupied port 3333. |
Description
The port handling logic was flawed added proper code to verify and assign ports with a logic for retries as well.
Also changed the debugger tab so that the gdb client controls are only enabled when the openOCD server is not started from the IDE.
Fixes # (IEP-1265)
Type of change
Please delete options that are not relevant.
How has this been tested?
Try debugging on mac os and verify that openocd is able to connect. Also start openocd only from cli and try to connect debugger
Test Configuration:
Checklist
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features