Community
Participate
Working Groups
Created attachment 286402 [details] OK Case We observe the following behaviour we run (larger) file search and replace operations using the search dialog. the following build sometimes is invisible to the user as it runs in the background (system) job looking at the code i can see that SearchPlugin.setAutoBuilding(false|isAutobuilding) is called by the SearchDialog. now it depends when at org.eclipse.core.internal.events.AutoBuildJob.build(AutoBuildJob.java:102) at org.eclipse.core.internal.events.BuildManager.endTopLevel(BuildManager.java:596) at org.eclipse.core.internal.resources.Workspace.endOperation(Workspace.java:1517) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2306) is called so that it will set the autobuilds state to system. in the bad scenario it will look like SearchPlugin.setAutoBuilding false set System: true via java.lang.Exception at org.eclipse.core.internal.events.AutoBuildJob.build(AutoBuildJob.java:102) at org.eclipse.core.internal.events.BuildManager.endTopLevel(BuildManager.java:596) at org.eclipse.core.internal.resources.Workspace.endOperation(Workspace.java:1517) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2306) at org.eclipse.core.internal.resources.Workspace.run(Workspace.java:2317) at org.eclipse.search2.internal.ui.text.MarkerHighlighter.addHighlights(MarkerHighlighter.java:49) at org.eclipse.search2.internal.ui.text.EditorAnnotationManager.addAnnotations(EditorAnnotationManager.java:244) at org.eclipse.search2.internal.ui.text.EditorAnnotationManager.searchResultChanged(EditorAnnotationManager.java:131) at org.eclipse.search.ui.text.AbstractTextSearchResult.fireChange(AbstractTextSearchResult.java:265) at org.eclipse.search.ui.text.AbstractTextSearchResult.addMatches(AbstractTextSearchResult.java:108) at org.eclipse.search.internal.ui.text.FileSearchQuery$TextSearchResultCollector.flushMatches(FileSearchQuery.java:211) at org.eclipse.search.internal.ui.text.FileSearchQuery$TextSearchResultCollector.acceptFile(FileSearchQuery.java:84) at org.eclipse.search.internal.core.text.TextSearchVisitor$TextSearchJob.processFile(TextSearchVisitor.java:216) at org.eclipse.search.internal.core.text.TextSearchVisitor$TextSearchJob.run(TextSearchVisitor.java:188) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:63) SearchPlugin.setAutoBuilding true // here is no build call so the following auto build jobs is a system SomeBuilder: Auto build... (P/someProject)
Created attachment 286403 [details] BAD case
I see that the code in org.eclipse.search.internal.ui.SearchPlugin.setAutoBuilding(boolean) is always called during search operation and it switches autobuild off/on, and that happens since very first public commit. So every build triggered during the search automatically converts itself into a "silent system" build. Not sure why that should be needed (avoid search errors on generated files?). However, search plugin is not the only place where one could switch auto build off and auto build would be still triggered. So fixing that is probably not the right solution. Now I also know why the build on startup with Oomph based installations is very often "silent" - because Oomph also switches autobuild off/on during startup tasks execution, any resource change that triggers the build automatically switches it to the "system" state. I see that org.eclipse.core.internal.events.BuildManager.endTopLevel(boolean) unconditionally triggers autoBuildJob.build(needsBuild) and that one sets the job state to the opposite of the *current* "autobuild" flag state: setSystem(!isAutoBuilding). The problem with this is that Job.setSystem() *must* be called *before* scheduling the job, so at the time where the autobuild job is *really* executed after the delay, and the "autobuild" flag is back to "true", we still run the build as a system one. This is questionable. I wonder what is the point of specifying the system state for builds triggered during the "autobuild off" state, if the actual build always happens only if the "autobuild" flag is turned back to "true". The "system" flag is coming from bug 47710 / 3.0 that was supposed to not show "building" jobs that appear during autobuild is off. They appeared in the progress view because they check if they should do anything inside the job run() method. One solution would be to check if the "autobuild" is on or off at run() and in case the value doesn't match our state, re-schedule the job without starting the build, so the system flag is properly set. On the other side, I wonder, why do we schedule() a build job at all if we already know it would not do anything because "autobuild" is off? Shouldn't we simply don't schedule the job if "autobuild" is off? Note: it will be *automatically* re-scheduled on switching "autobuild" on again.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/180722
(In reply to Eclipse Genie from comment #3) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/180722 This is quite a change, I haven't tested if that works or not, but the main idea is simply to *NOT* schedule autobuild job at all if autobuild is disabled - and this way get rid of "system" flag completely. Now if autobuild job is *really* doing something, it runs always as non-system job and is always shown in the Progress view. I honestly have no idea why AutoBuildJob was implemented in the way where it is *always* executed but inside the run() decides not to do anything if autobuild is disabled.
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/181494
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/181494 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=36b7013aa78c11857bc824b7436fc3de5c0d245a
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/180722 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=4bca749b780c2a9221c9614490e636fe5d2de99b
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/181688
(In reply to Eclipse Genie from comment #8) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/181688 This is a revert of the patch, that caused bug 574098. I'm investigateing what the JDT tests are doing there and what is missing in the original patch.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/181688 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=825390a1226648c042da6846d79ba335fae5e82d
The ClasspathTests fail because they don't see a job of the family AUTO_BUILD and they don't see IResourceChangeEvent.PRE_BUILD/POST_BUILD events, both *if autobuild is disabled*! How bizarre is this? ClasspathTests disable autobuild, the tests in question do not enable it, but still rely on AUTO_BUILD job and build events sent during the "no build" run of the AutoBuild job. OMFG. This patch on top of original change "fixes" ClasspathTests: diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/AutoBuildJob.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/AutoBuildJob.java index a3be0ad..d626007 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/AutoBuildJob.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/events/AutoBuildJob.java @@ -78,2 +78,4 @@ + Job fake; + /** @@ -102,2 +104,28 @@ schedule(delay); + } else { + if (fake == null) { + fake = new Job("Faked Autobuild") { //$NON-NLS-1$ + @Override + public boolean belongsTo(Object family) { + return family == ResourcesPlugin.FAMILY_AUTO_BUILD; + } + + @Override + protected IStatus run(IProgressMonitor monitor) { + try { + final int trigger = IncrementalProjectBuilder.AUTO_BUILD; + workspace.broadcastBuildEvent(workspace, IResourceChangeEvent.PRE_BUILD, trigger); + workspace.broadcastBuildEvent(workspace, IResourceChangeEvent.POST_BUILD, trigger); + } catch (Exception e) { + // + } + return Status.OK_STATUS; + } + + }; + fake.setRule(workspace.getRoot()); + } + if (fake.getState() != Job.RUNNING) { + fake.schedule(delay); + } } Sure, failed ClasspathTests are a direct side effect of the change - if autobuild is off, we will not do *anything* now, as expected, and the patch above shows what the ClasspathTests are missing. So we have code relying on any job of AUTO_BUILD family executed and build events sent after resource changes with *autobuild off*. Questions are: 1) Is this a valid expecation? Most likely not would be my naive assumption, but ... Look at bug 75607 comment 2. It refers to the IResourceChangeEvent javadoc that says .. tada ... "If autobuilding is not enabled, these events still occur at times when autobuild would have occurred". OMG. Already 2004 this was considered as "legacy", see bug 77312 comment 0. 2) How many such "crazy" clients do we have? Probably not many, but who knows. 3) How often is this the use case, that autobuild is disabled in the IDE but still someone expects some kind of a build to happen? Depends on 2), and affects probably only people that use IDE with autobuild switched off. That may probably affect C++ people who tend to disable autobuild and run explicit builds. Possible ways to deal with this: 1) Fix ClasspathTests and hope that that is it. 2) "mimic" the old behavior by running a "dummy" system job that does nothing except sending those stupid build events? 3) Add a system property to keep the old behavior, including the behavior caused *this* bug? 4) Add a system property to generate "dummy" events only? 5) "mimic" the old behavior by running a "dummy" system job and add a system property to disable that? 6) Anything else? Originally I wanted propose 1 + 4. Try to fix ClasspathTests and provide a way for affected clients to restore previous state. Reasoning: - "Autobuild off" is not default. This reduces the number of affected clients. - Code that relies on build events during "Autobuild off" is bizarre, probably not much code affected here too. - Most clients expect *nothing* to happen with switched autobuild - that is the reason they switch it off. The point that we still send workspace build events if autobuild is off could be considered as a bug. - We've learned many times that there are enough bizarre code in the universe. This behavior here exists since >10 years, so I assume that we still need the possibility to restore to the original behavior. But after stumbling over bug 75607 comment 2 and IResourceChangeEvent javadoc I see that this approach would break API contract that was written in stone since beginning of Eclipse. So now I would say 1) + 2) or just 2) - try to fix ClasspathTests and mimic the old behavior by sending those events in cases where autobuild is off. In the ideal world I would change IResourceChangeEvent spec and stop doing bizarre build notifications if build isn't running at all. Any comments?
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/181719
(In reply to Eclipse Genie from comment #12) > New Gerrit change created: > https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/181719 That is the updated proposal.
Gerrit change https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/181719 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.resources.git/commit/?id=6f51e1b87d52320dceed8ee115898b8d43e91f1a
We just noted, that AutoBuildOffJob processes resourceChanged notification to handle classpathchanges. We do not have any clue if that is good or bad, but it feels wrong: Thread [Worker-11: Sending build events with disabled autobuild] (Suspended (breakpoint at line 2375 in org.eclipse.jdt.internal.core.ClasspathEntry)) org.eclipse.jdt.internal.core.ClasspathEntry.validateLibraryEntry(org.eclipse.core.runtime.IPath, org.eclipse.jdt.core.IJavaProject, java.lang.String, org.eclipse.core.runtime.IPath, java.lang.String, boolean) line: 2375 org.eclipse.jdt.internal.core.ClasspathEntry.validateClasspathEntry(org.eclipse.jdt.core.IJavaProject, org.eclipse.jdt.core.IClasspathEntry, org.eclipse.jdt.core.IClasspathContainer, boolean, boolean) line: 2295 org.eclipse.jdt.internal.core.ClasspathEntry.validateClasspathEntry(org.eclipse.jdt.core.IJavaProject, org.eclipse.jdt.core.IClasspathEntry, org.eclipse.jdt.core.IClasspathContainer, boolean, boolean) line: 2236 org.eclipse.jdt.internal.core.ClasspathEntry.validateClasspathEntry(org.eclipse.jdt.core.IJavaProject, org.eclipse.jdt.core.IClasspathEntry, boolean, boolean) line: 2166 org.eclipse.jdt.internal.core.ClasspathValidation.validate() line: 75 org.eclipse.jdt.internal.core.DeltaProcessor.resourceChanged(org.eclipse.core.resources.IResourceChangeEvent) line: 2199 org.eclipse.jdt.internal.core.DeltaProcessingState.resourceChanged(org.eclipse.core.resources.IResourceChangeEvent) line: 501 org.eclipse.core.internal.events.NotificationManager$1.run() line: 305 org.eclipse.core.runtime.SafeRunner.run(org.eclipse.core.runtime.ISafeRunnable) line: 45 org.eclipse.core.internal.events.NotificationManager.notify(org.eclipse.core.internal.events.ResourceChangeListenerList$ListenerEntry[], org.eclipse.core.internal.events.ResourceChangeEvent, boolean) line: 295 org.eclipse.core.internal.events.NotificationManager.broadcastChanges(org.eclipse.core.internal.watson.ElementTree, org.eclipse.core.internal.events.ResourceChangeEvent, boolean) line: 158 org.eclipse.core.internal.resources.Workspace.broadcastBuildEvent(java.lang.Object, int, int) line: 367 org.eclipse.core.internal.events.AutoBuildJob$AutoBuildOffJob.run(org.eclipse.core.runtime.IProgressMonitor) line: 319 org.eclipse.core.internal.jobs.Worker.run() line: 63
(In reply to Jörg Kubitz from comment #15) > We just noted, that AutoBuildOffJob processes resourceChanged notification > to handle classpathchanges. We do not have any clue if that is good or bad, > but it feels wrong Please read comment 11.
(In reply to Andrey Loskutov from comment #16) > (In reply to Jörg Kubitz from comment #15) > > We just noted, that AutoBuildOffJob processes resourceChanged notification > > to handle classpathchanges. We do not have any clue if that is good or bad, > > but it feels wrong > > Please read comment 11. I did, i understand you did not change the behaviour, it just was not so obvious to see before. One consequence is that Autobuild turned off still locks the workspace. Shortly - but still - it does.
