Bugzilla – Full Text Bug Listing
| Summary: | RCS job locks some random project when scanning for repository changes | ||
|---|---|---|---|
| Product: | [Technology] EGit | Reporter: | Remy Suen <remy.suen> |
| Component: | UI | Assignee: | Project Inbox <egit.ui-inbox> |
| Status: | CLOSED FIXED | QA Contact: | |
| Severity: | major | ||
| Priority: | P3 | CC: | jens.baumgart, mathias.kinzler, robin.rosenberg |
| Version: | unspecified | ||
| Target Milestone: | 0.8.0 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
I think it is wise to do as little work as possible in the listeners, but this applies to any listern. That is why most event handlers in other context do not do a lot of work, but rather spawn off a new task as quickly as possible. That aspect of events is not tied to RepositoryListeners. The RCS job could probably be implemented better wrt locking anyway. The general idea is not having to wait as long as the regular automatic workbench refresh, but rather something closer to the time needed from performing a git command in a shell and then switching back to Eclipse. On thing in particular that I did not yet implement is to use index changes to be able to scan only a subset of the workspace. Hi Remy, since you usually do not know which locks are held by the calling thread it is problematic to aquire locks in a listner. Doing the work in a Job solves this issue (but of course you might get other issues because work is done later on a possibly changed state)). (In reply to comment #2) > since you usually do not know which locks are held by the calling thread it is > problematic to aquire locks in a listner. Yes, I've altered my code to use a job instead so this is not really a big deal for me. I've changed the summary to reflect the problem where the RCS job locks a single random project. (In reply to comment #1) > The RCS job could probably be implemented better wrt locking anyway. > The general idea is not having to wait as long as the regular automatic > workbench refresh, but rather something closer to the time needed from > performing a git command in a shell and then switching back to Eclipse. On > thing in particular that I did not yet implement is to use index changes to be > able to scan only a subset of the workspace. 1. I think the current implementation locks wrongly: if a repo has projects p1, p2, p3 then p1 is locked and the repo is scanned. All projects of the repo should be locked when scanning. 2. Is the lock needed at all? Repository.scanForRepoChanges() does not scan the working tree. Why is a resource lock required here? Fixed with http://egit.eclipse.org/r/#change,783 Repo scan is now done without resource locks. Merged as f36a6bcfab03302411c5252d01e969b3d305cbe8 |
The RCS job performs a scan for repository changes every 10 seconds. If there are changes, the repository will fire off a RefsChangedEvent to its listeners. These listeners could theoretically do anything, including wanting to acquire a lock on some projects. However, this is not possible because the jobs framework will detect that you are trying to acquire a lock outside of the lock that's been acquired by the RCS job. Please see the run(IStatus) method of the Activator.RCS class in the org.eclipse.egit.ui bundle. ------------------ // where 'p' is some random project that is versioned under the repository 'r' ISchedulingRule rule = p.getWorkspace().getRuleFactory().modifyRule(p); getJobManager().beginRule(rule, monitor); try { r.scanForRepoChanges(); } finally { getJobManager().endRule(rule); } ------------------ You can reproduce the problem by using some code like the following in a view that's up during workbench restart (which ought to ensure that the listeners will be attached before the RCS job is run). public void createPartControl(Composite parent) { for (IProject project : ResourcesPlugin.getWorkspace().getRoot() .getProjects()) { RepositoryMapping mapping = RepositoryMapping.getMapping(project); if (mapping != null) { mapping.getRepository().addRepositoryChangedListener( new RepositoryAdapter() { @Override public void refsChanged(RefsChangedEvent e) { lock(e.getRepository()); } }); } } } private void lock(Repository repository) { Set<IProject> projects = new HashSet<IProject>(); for (IProject project : ResourcesPlugin.getWorkspace().getRoot() .getProjects()) { RepositoryMapping mapping = RepositoryMapping.getMapping(project); if (mapping != null && repository == mapping.getRepository()) { projects.add(project); } } IWorkspaceRunnable runnable = new IWorkspaceRunnable() { public void run(IProgressMonitor monitor) throws CoreException { } }; try { ResourcesPlugin.getWorkspace().run(runnable, new MultiRule(projects.toArray(new IProject[projects.size()])), IWorkspace.AVOID_UPDATE, null); } catch (CoreException e) { // TODO Auto-generated catch block e.printStackTrace(); } } java.lang.IllegalArgumentException: Attempted to beginRule: MultiRule[P/org.eclipse.egit.ui.test,P/org.eclipse.egit-updatesite,P/org.eclipse.egit-feature], does not match outer scope rule: P/org.eclipse.egit-feature at org.eclipse.core.runtime.Assert.isLegal(Assert.java:63) at org.eclipse.core.internal.jobs.ThreadJob.illegalPush(ThreadJob.java:136) at org.eclipse.core.internal.jobs.ThreadJob.push(ThreadJob.java:326) at org.eclipse.core.internal.jobs.ImplicitJobs.begin(ImplicitJobs.java:63) at org.eclipse.core.internal.jobs.JobManager.beginRule(JobManager.java:285) at org.eclipse.core.internal.resources.WorkManager.checkIn(WorkManager.java:117) at org.eclipse.core.internal.resources.Workspace.prepareOperation(Workspace.java:1914) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:1970) at org.eclipse.egit.ui.internal.staging.TestView.lock(TestView.java:69) at org.eclipse.egit.ui.internal.staging.TestView.access$0(TestView.java:54) at org.eclipse.egit.ui.internal.staging.TestView$1.refsChanged(TestView.java:47) at org.eclipse.jgit.lib.Repository.fireRefsChanged(Repository.java:1286) at org.eclipse.jgit.lib.RefDirectory.fireRefsChanged(RefDirectory.java:834) at org.eclipse.jgit.lib.RefDirectory.getRefs(RefDirectory.java:262) at org.eclipse.jgit.lib.Repository.getAllRefs(Repository.java:998) at org.eclipse.jgit.lib.Repository.scanForRepoChanges(Repository.java:1310) at org.eclipse.egit.ui.Activator$RCS.run(Activator.java:320) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54) ------------------ I'll probably workaround the problem on my end by scheduling another job to do my work in response to the RefsChangedEvent I guess.