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.

Sign up for GitHub

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

Jump to bottom

synchronise resource tree modification #459

Merged
merged 1 commit into from Apr 4, 2023
Merged

synchronise resource tree modification #459

merged 1 commit into from Apr 4, 2023

Conversation

gireeshpunathil
Copy link
Contributor

SearchablePluginsManager saves the state of the resources in the workspace without acquiring a lock, or not scheduling the job asynchronously.

Fix it by employing the WorkspaceJob scheduler.

Fixes: #64

@github-actions
Copy link

github-actions bot commented Feb 6, 2023 •
edited

Test Results

     80 files  ±0       80 suites  ±0   14m 0s ⏱️ - 3m 30s
3 278 tests ±0  3 254 ✔️ ±0  24 💤 ±0  0 ❌ ±0 
3 377 runs  ±0  3 353 ✔️ ±0  24 💤 ±0  0 ❌ ±0 

Results for commit 4bf3e60. ± Comparison against base commit 91ab862.

♻️ This comment has been updated with latest results.

@gireeshpunathil gireeshpunathil marked this pull request as ready for review February 7, 2023 11:58
@gireeshpunathil
Copy link
Contributor Author

I have made a number of assumptions in this PR, which I would like to be validated as part of the review. thanks in advance to whover is going to do that!

  • workspace-wide resource modification is delegated to org.eclipse.core.resources.WorkspaceJob. Is this the right job for this type of work?
  • the rule of this job is set to be the root of the resource, following how it is done elsewhere in the code. Is there a place where it defines what type of rules apply to what type of jobs?
  • to bring up the progressive status, I am using Display class, which introduces a new bundle-lvele dependency with oreg.eclipse.ui. Is it normal, safe, and non-breaking? do I need to worry about this anywhere - such as bumping the plugin version or something?

@vik-chand
Copy link
Member

@iloveeclipse can you please review this pr?

@merks
Copy link
Contributor

merks commented Feb 7, 2023

The core should not depend on the UI!

I think you could use one of these instead of scheduling a job:

org.eclipse.core.resources.IWorkspace.run(ICoreRunnable, ISchedulingRule, int, IProgressMonitor)
org.eclipse.core.resources.IWorkspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor)

org.eclipse.pde.internal.core.BundleValidationOperation uses that approach.

@iloveeclipse
Copy link
Member

@iloveeclipse can you please review this pr?

Will try, have few other PR's in the review list.

@gireeshpunathil
Copy link
Contributor Author

@merks @iloveeclipse - pushed new change with the approach suggested, pls have a look; thanks!

@laeubi laeubi requested a review from iloveeclipse March 9, 2023 08:24
iloveeclipse
iloveeclipse previously requested changes Mar 9, 2023
View reviewed changes
Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General note: looks now better in code, haven't validated if the problem disappears.

  • Please rebase on latest master state (do not merge!)
  • Please bump last bundle version segment with +100.

ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/SearchablePluginsManager.java Show resolved Hide resolved
ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/SearchablePluginsManager.java Show resolved Hide resolved
iloveeclipse
iloveeclipse reviewed Mar 10, 2023
View reviewed changes
Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't validated, but my questions were answered now, so should in theory work.

@gireeshpunathil @vik-chand
synchronize resource tree modification
4bf3e60 
SearchablePluginsManager saves the state of the
resources in the workspace without acquiring a
lock, or not scheduling the job asynchronously.
Fix it by employing the ICoreRunnable scheduler.
@gireeshpunathil
Copy link
Contributor Author

there are not outstanding review comments, so this should be ready to merge, has been in this state for a while.

@iloveeclipse iloveeclipse merged commit 71e89bb into eclipse-pde:master Apr 4, 2023
5 of 7 checks passed
@iloveeclipse
Copy link
Member

there are not outstanding review comments, so this should be ready to merge, has been in this state for a while.

Sorry, too many bugs & reviews...

@jukzi
Copy link
Contributor

jukzi commented Apr 4, 2023

@gireeshpunathil thanks for fixing, @iloveeclipse thanks for review

@laeubi laeubi added this to the 4.28 M1 milestone Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iloveeclipse iloveeclipse iloveeclipse left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.28 M1
Development

Successfully merging this pull request may close these issues.

java.lang.Exception: The resource tree is locked for modifications.
6 participants
@gireeshpunathil @vik-chand @merks @iloveeclipse @jukzi @laeubi