IEP-928: Workspace hangs if project is closed while indexer is running #812
Conversation
|
Hi @sigmaaa I don't see any issues with approach, let @AndriiFilippov verify multiple projects and scenarios. Can you list down the dependent components which he could check. |
Hi Kondal, thanks for the review. I've provided dependent components in the description |
|
@sigmaaa hi ! Tested under: I don't see any issues when closing the project during indexing, but I notice there is error once I cancel build. Steps:
|
WalkthroughThe changes introduced in this pull request primarily focus on improving the performance of the IDFBuildConfiguration class by processing compile commands asynchronously. This is achieved by using Eclipse's Job and JobGroup classes to handle the parsing task in a separate thread, thereby enhancing the responsiveness of the main application. Changes
TipsChat with CodeRabbit Bot (
|
sigmaaa
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (3 hunks)
Additional comments (Suppressed): 3
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (3)
85-97: The new imports
org.eclipse.core.runtime.IStatus,org.eclipse.core.runtime.Status, andorg.eclipse.core.runtime.jobs.JobGroupare necessary for the changes made in this PR. They are used to create a new JobGroup and handle its status.130-130: A new
JobGroupis created with a limit of one job at a time. This ensures that only one parsing job runs at any given time, which can help prevent potential race conditions or resource contention issues.private JobGroup jobGroup = new JobGroup("Parsing Job...", 1, 1); //$NON-NLS-1$
- 974-997: The method
processCompileCommandsFilehas been modified to run the parsing process asynchronously in a separate job. This change allows the parsing process to be interrupted without causing the entire workspace to hang. However, it's important to ensure that any code that depends on the completion of the parsing process is properly synchronized or otherwise handles the asynchronous nature of this operation.Job parseJob = new Job("Parse Compile Commands File") //$NON-NLS-1$ { protected IStatus run(IProgressMonitor monitor) { try { parser.parse(monitor); } catch (CoreException e) { Logger.log(e); } return Status.OK_STATUS; } }; parseJob.setJobGroup(jobGroup); parseJob.schedule();
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (3)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (3 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java (1 hunks)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties (1 hunks)
Files skipped from review due to trivial changes (2)
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/Messages.java
- bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/messages.properties
Additional comments (Suppressed): 2
bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java (2)
88-94: The new imports
IStatus,Status, andJobGroupare introduced to support the creation of a new Job for parsing the compile commands file asynchronously. This is in line with the PR's goal of preventing workspace hang by running the indexer in a separate thread.130-130: A new
JobGroupinstance is created. TheJobGroupclass is part of the Eclipse Jobs API and is used to group related jobs together. Here, it's used to manage the job that parses the compile commands file. Ensure that thisJobGroupis properly managed elsewhere in the codebase, i.e., jobs are correctly added and removed when necessary.
| private void processCompileCommandsFile(IConsole console, IProgressMonitor monitor) throws CoreException | ||
| { | ||
| IFile file = getCompileCommandsJsonFile(monitor); | ||
|
|
||
| CompileCommandsJsonParser parser = new CompileCommandsJsonParser( | ||
| new ParseRequest(file, new CMakeIndexerInfoConsumer(this::setScannerInformation, getProject()), | ||
| CommandLauncherManager.getInstance().getCommandLauncher(this), console)); | ||
| parser.parse(monitor); | ||
| Job parseJob = new Job(Messages.IDFBuildConfiguration_ParseCommand) | ||
| { | ||
| protected IStatus run(IProgressMonitor monitor) | ||
| { | ||
| try | ||
| { | ||
| parser.parse(monitor); | ||
| } | ||
| catch (CoreException e) | ||
| { | ||
| Logger.log(e); | ||
| } | ||
| return Status.OK_STATUS; | ||
| } | ||
| }; | ||
| parseJob.setJobGroup(jobGroup); | ||
| parseJob.schedule(); | ||
| } |
There was a problem hiding this comment.
The processCompileCommandsFile method has been modified to create a new Job for parsing the compile commands file asynchronously. This change is consistent with the PR's goal of preventing workspace hang by running the indexer in a separate thread. However, there's no error handling mechanism if the job fails or doesn't complete successfully. Consider adding an appropriate error handling mechanism.
+ catch (CoreException e)
+ {
+ Logger.log(e);
+ return Status.CANCEL_STATUS;
+ }
Description
Workspace hangs sometimes if we try to close the project while the indexer is running. Fixed by encapsulating the parsing process into separate Job.
Fixes # (IEP-928)
Type of change
Please delete options that are not relevant.
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit