Community
Participate
Working Groups
RC1 - lock workspace in background operation - open installed JREs - add a JRE - press OK observe: nothing happens. There should be a blocking dialog informing me about the conflict with the background operation. What is dangerous in this scenario is the fact that I can cancel the dialog and starting another operation which basically stacks operations.
The code is calling PlatformUI.getWorkbench().getProgressService ().busyCursorWhile() which is the correct thing to do. Need to track down why busyCursorWhile() doesn't show the progress dialog. Don't change your code until we understand the issue with busyCursorWhile().
The problem is that the ProgressDialog doesn't show up. IProgressService.busyCursorWhile() defers the appearance of the progress dialog until no more modal dialog is visible. However, in this scenario busyCursorWhile() is called from a modal dialog. This looks like a similar problem that existed in runInUI when called from a modal dialog. Moving to platform-ui and increasing the severity since you can easily generate exceptions once you are in this state.
IProgressService.busyCursorWhile() is speced to not open another dialog if there is one already open - we would break API if we made any change like that. This was a basic design goal of this support as we do not want to block a request made from a running job. I don't think you should call busyCursorWhile from a modal dialog as we can't make the right decisions about disabling buttons etc.
Adding Erich for comment.
I think we need to be more explicit in the javadoc about the issues with running this from a modal dialog.
*** Bug 64925 has been marked as a duplicate of this bug. ***
*** Bug 65024 has been marked as a duplicate of this bug. ***
Making the javadoc more explicit doesn't fully address this issue. 1) The "Showing Modal Progress" document is wrong. It recommends to use IProgressService.busyCursorWhile() when the operation can run in the forked=true mode. The document can be fixed as well, but the progress migration story is getting more complex. 2) IProgressService.run() which implements IRunnableContext.run() has the same constraint when fork=true. This violates the API contract from IRunnableContext.run() (it makes the precondition stronger). So when it comes to use IProgressService from a Modal Dialog then our story is: run() - no, runInUI() - yes, busyCursorWhile() - no. This isn't consistent. Is there a way to make this more consistent. For example, by distinguishing whether: 1) busyCursorWhile() was called from a modal dialog 2) a modal dialog is up when the ProgressDialog is about to be shown after IProgressService.getLongOperationTime().
Whatever we do we should do it by RC2 so I am marking as such. We have the following issues 1) If called from a modal dialog we cannot open if someone closes that dialog in the meantime - the parent would be null. As we cannot reparent a dialog without an API change at this stage we would have to just continue with the busy cursor and not show the dialog. 2) We have to know if the current modal dialog is the one that was open when we were called. This is just a case of caching the modal shell. I am reluctant to go back allowing these dialogs to open on a modal shell as this is a challenge to get right - it also breaks our rule of trying to avoid multiple modal dialogs opened. However I think what might be better is to allow them to open a blocked dialog in this case like in runInUI - for instance if I did: Platform.getJobManager().beginRule(null, new EventLoopProgressMonitor(new NullProgressMonitor ()))); Opinions? I think the lack of blocked notification is the bigger problem here.
I just had a look at the code and in JREPreferencePage and it calls the code below. Are you doing this somewhere else? BusyIndicator.showWhile(null, new Runnable() { public void run() { IVMInstall defaultVM = getCurrentDefaultVM(); IVMInstall[] vms = fJREBlock.getJREs(); JREsUpdater updater = new JREsUpdater(getShell ()); if (!updater.updateJRESettings(vms, defaultVM)) { canceled[0] = true; } } });
This code is executed from anywhere the JREs pref page can be accessed - i.e. the standard pref pages, from "Installed JREs..." button on the JRE tab of a launch config, and the "define system library" quick fix (there may be others). However, it all uses the same code.
Adding a JRE does not use this API in my install of RC1. Search does (but it doesn't have this problem). My steps are lock workspace in background operation - open installed JREs preference page - add a JRE - press OK and the code I pasted from JREPreferencePage gets called. I really need a better pointer and better steps to be sure this is the same code because right now this looks unrelated. Has it changed in HEAD since RC1?
Debug has not changed since RC1 (i.e. no changes in HEAD). Dirk originally filed this problem (thru me, and bug 64928), so perhaps he can enlighten us on the test case.
YOu have two press OK to times in the end. First on the dialog adding the JRE and then on the preference dialog. When pressing OK on the preference dialog the dialog doesn't disappear and the cursor turns into a busy cursor. However the dialog is still enabled and I can easily press other controls.
The test case is correct, Tod is correct, my initial analysis wasn't correct... The problem isn't caused by IProgressService.busyCursorWhile() (I'll work on a test case for this scenario). The issue is that the blocked jobs dialog doesn't appear after pressing OK for the first time, that is, the user shouldn't be able to press OK a second time. I've attached the stack trace below, the operation is executed with a NullProgressMonitor which is then wrappered into an EventLoopProgressMonitor. So let's continue from comment#9. A solution similar to the one used in runInUI would be desirable. Thread [main] (Suspended) Object.wait(long) line: not available [native method] ThreadJob.joinRun(IProgressMonitor) line: 159 ImplicitJobs.begin(ISchedulingRule, IProgressMonitor, boolean) line: 87 JobManager.beginRule(ISchedulingRule, IProgressMonitor) line: 170 WorkManager.checkIn(ISchedulingRule, IProgressMonitor) line: 95 Workspace.prepareOperation(ISchedulingRule, IProgressMonitor) line: 1628 Workspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor) line: 1668 SetClasspathOperation(JavaModelOperation).runOperation (IProgressMonitor) line: 744 JavaProject.setRawClasspath(IClasspathEntry[], IPath, IProgressMonitor, boolean, IClasspathEntry[], boolean, boolean) line: 2655 JavaCore$3.run(IProgressMonitor) line: 3433 BatchOperation.executeOperation() line: 34 BatchOperation(JavaModelOperation).run(IProgressMonitor) line: 700 Workspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor) line: 1673 JavaCore.run(IWorkspaceRunnable, ISchedulingRule, IProgressMonitor) line: 3246 JavaCore.setClasspathContainer(IPath, IJavaProject[], IClasspathContainer[], IProgressMonitor) line: 3416 JREContainerInitializer.initialize(IPath, IJavaProject) line: 51 LaunchingPlugin$VMChanges.rebind() line: 250 LaunchingPlugin$VMChanges.access$0(LaunchingPlugin$VMChanges) line: 214 LaunchingPlugin$1.run(IProgressMonitor) line: 204 BatchOperation.executeOperation() line: 34 BatchOperation(JavaModelOperation).run(IProgressMonitor) line: 700 Workspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor) line: 1673 JavaCore.run(IWorkspaceRunnable, ISchedulingRule, IProgressMonitor) line: 3246 LaunchingPlugin$VMChanges.process() line: 207 LaunchingPlugin.processVMPrefsChanged(String, String) line: 586 LaunchingPlugin.propertyChange(Preferences$PropertyChangeEvent) line: 493
I have logged Bug 65277 about the preference page so that we can concentrate on the busyCursorWhile issue in this discussion. There is still one thing that would be an issue if we used the same solution as runInUI - namely that you could try and open the progress dialog on a disposed shell. It is somewhat more acute in this case as delay the opening of the dialog for 250ms which is ample time for the parent to be destroyed. I think the only safe solution is to open on the shell we start with and only block on different shells. The issue is what to do if that shell is disposed in the meantime. If the API freeze hadn't happened we could just reparent (Dialogs don't allow reparenting currently). As we can't do that I could a) Cache the starting shell and change getParentShell() to make the right decision - i.e. not return the parent if it is disposed. This is a little risky but I think it can be done without serious breakage. I would add a check to see if the modal shell open was the parent and not block if so. b) Never open the progress dialog if the parent is disposed - leave it as the busy cursor. I think a) is likely the only acceptable solution. Opinions?
Just to summarize where we are at. There are two issues: 1) when executing an operation from a dialog with a null progress monitor then the blocked jobs dialog doesn't show up (this is the original problem of this bug report). This one is now tracked in bug# 65277 2) when calling IProgressService.busyCursorWhile() from a modal dialog (PreferenceDialog, PropertiesDialog, or a custom dialog) then the progress dialog doesn't show up. I could verify this behaviour in a test case. This issue is tracked in this bug report. I've changed the bug title to match the discussion. Continuing with problem 2). The API freeze is real, but the PMC can approve API additions. What would the API addition be and how would it simplify the solution. I agree that b) isn't an acceptable solution, but if an API addition makes the implementation of a) simpler and more robust then we should pursue this path.
1) The case in Bug 65277 is unrelated to the progress support. It is just a safety check for the preference dialog. The oroblem we have now is that we do not ever open when something is not using the progress API and a modal dialog is open. I think we won't want to change that. 2) I'm not sure that we need to ask for an API change - I can infer the open modal dialog as well as have it passed in. The issue with the Dialog class is not a general problem so I could always copy code down and refactor post 3.0.
Stefan has come up with a much better solution. The issue we have is that we don't know if the modal dialog that is up is something that is blocking the progress or not. Inferring something from the dialog is hard to do as there are no pointers to our code. If we infer instead based on ticks from the progress we can know if we are blocked or not - that is if we get a worked() call at any point in that 800 ms we know that progress is not blocked and we can open the progress dialog. Likewise if showBlocked is called on the dialog we know that it can't do anything yet and can open it. If not then it is blocked on user input and we should wait. This relies on users of this API to update thier ticks when possible but that it a good idea anyways.
>1) The case in Bug 65277 is unrelated to the progress support. It is just a >safety check for the preference dialog. The oroblem we have now is that we do >not ever open when something is not using the progress API and a modal dialog >is open. I think we won't want to change that. I don't like that we cannot provide more support to make this better, but I have to agree with you. The real problem here is that clients are using null progress monitors when they shouldn't. We the new responsiveness work almost any operation can be long running since it can block. Filed a separate bug for the original issue bug# 65504.
Here is another idea for how to solve this problem... Make the ProgressMonitorJobsDialog API. This would allow the client to pass in the parent shell. + no new code in a sensible area - an API addition (I would bring this forward to the rest of the PMC) - you don't get the delayed dialog appearance (which I would tolerate given where we are at).
But then that defeats the purpose of busyCursorWhile - that is that you can a busy cursor if your operation is less than 800ms. If we open imemdiately it is no different that ProgressMonitorDialog.run() except that you will get the ProgressMonitorJobsDialog instead. I think I would like to take Stefans idea and see how it plays - I think the code change is hopefully low risk.
If you consider the code change low risk then I also prefer to go for the full solution and supporting a delayed appearance.
I will attach a suggested patch. I have tested it against the JRE preference page Search button. Now if you open a progress monitor jobs dialog and ask it if it ticking I set a flag where I track it and try again in 800 ms. If it gets a tick in that time (i.e. anything sent to the progress monitor) I will then open the dialog. There was also a bug where the dialog could be closed by the end of the job and then re-opened. I have included a fix for that by setting a flag on close that the rescheduling algorithm checks. I tested using the JUnitPlugin.createAllPackagesDialog method using the JUnit Preference Page. I added in a series of delays - no delay is fast enough that the reschedule check never happens - 50ms delay creates a reschedule check but the runnable is done. I check the closed dialog case with this - 100 ms delay will bring up the progress dialog. One thing I am tempted to do is to reschedule the opening sooner (maybe short operation rather than long operation time). Opinions? Erich if you could apply this patch and let me know if it solves your problem that would be very much appreciated.
Created attachment 11591 [details] Patch for busyCursorWhile
Here is what I'm observing when running with the patch. The testcase is to execute a runnable with busyCursorWhile() from the PropertiesDialog.performOK() method. 1) workspace isn't locked - press OK - the jobs progress dialog appears after a delay ->this is the expected behaviour 2) lock the workspace - press OK ->only the busy cursor shows up, no progress dialog is shown and I'm not informed about the blockage. Case 2) is not what I would have expected/wished. I would have wished that the ProgressDialog appears (immediatly) with the details area expanded and I can cancel the blocking job consistent with our "user is in control" goal. This means a) we now get the delayed dialog appearance but b) we don't get the blockage information. To illustrate the behaviour I would like to get replace busyCursorWhile(r) with run(true, false, r) (which is a kludge, to get the progress dialog to appear immediatly).
Good idea Erich. I'll keep working on that case.
Erich there is no class PropertiesDialog and PropertyDialog does not have a performOK method. Is this a JDT thing? Either way could you attach a patch with your test case? I think I have what you need but I am not sure how to test it.
Created attachment 11606 [details] Test with the properties dialog I think this is the test condition you were talking about (PropertiesDialog#okPressed). When I applied the updated patch and tested this worked.
Created attachment 11607 [details] Updated patch to open on blocked
I have updated the patch. The test case I used was 1) Apply the patch 2) Start a job that locks the workbench 3) Open a properties dialog on a project 4) Modify the referenced projects 5) Hit save
Created attachment 11620 [details] plug-in with my test setup Winzip with test plug-in
I've attached my test setup in a plug-in above. The plugin contributes a property page "Progress Sample". Pressing OK start a long running operation. I'll verify the behaviour with your updated patch as well.
Tested with the updated patch, but I still see no progress dialog in the blocked case. This confuses me since our test cases are very similar. Here are my steps: 1) start a job that is locking the workspace from the JobFactory view 2) select a project 3) open Properties>Sample Progress 4) press OK ->busy cursor only
I have tested with your test plug-in and your steps are fine for me. However I had a problem applying this patch against HEAD so I suspect you might have been looking at a different state. I am updating the patch.
Created attachment 11657 [details] Updated patch
Erichs test case uncovered had a timing issue which I will update with a patch. If you setBlocked before the first tick was checked there would never be another one. The updated patch sets watching of ticks to true if you are blocked thereby always opening when blocked.
Created attachment 11663 [details] Updated patch
Patch submitted. John and I talked and there is still one concern about a possible problem case. If the following occurs: 1) Tick happens 2) Question Dialog is opened 3) Check the tick again and open progress dialog The progress dialog will cover the question dialog and you will have a deadlock. This is a pretty dangerous case and we should decide how to handle this i.e. do we document it or try some greater protection?
Comment 19 doesn't describe the algorithm I suggested. This was the suggestion: 1. Job starts. Take a timestamp. 2. Inside the handler for worked(), do this: { if (currentTime - initialTimestamp > 800ms) { open progress dialog } } This doesn't use any background jobs, timers, retries, or have any memory of when past ticks have occurred. The progress dialog is opened inside the worked() handler -- not in a background thread that checks if worked() has been called. Since worked() is called from the thread we're monitoring progress from, there are no concurrency issues.
Stefan's approach in comment #40 certainly sounds quite a lot simpler than the existing code. It avoids the possibility of multiple modal shell deadlock because it can't possibly be in a syncExec asking the user a question at the instant that it is reporting work. It would also fix bug 65951.
Stefan and I have gone through the deadlock case and have reworked the algorithm somewhat (and released it). Now we record the time of the first tick and open the dialog when a tick happens after 800 ms. This way a dialog opening in the meantime will block the tick and hence not open the dialog. By doing this via a callback from a tick rather than a polling from a tick we save ourselves having to keep state to check. I have also modified Erichs test case to open dialogs at some inconvenient moments. I will attach it for verfication.
Verified in 20040609 using the task dialog from an editor
I noticed that ProgressManager.busyCursorWhile is still scheduling a job to open the progress dialog. Is this needed in light of Stefan's fix to open the dialog from the worked callback? I am concerned because in some cases thousands of instances of this job were being created and flooding the job manager.
Yes it is - otherwise we are relying even more heavily on the user of the API to use the IProgressMonitor API. How is it flooding the job manager? If we are too aggressive we could look into that.
After retrying the scenario in bug 65415, it is no longer as bad. You have taken out the code that rescheduled the "Open Progress Monitor" job if a modal shell was open. So, there are are still 1500 instances of this job being created in this scenario, but they no longer reschedule themselves for the duration of the operation. It would be more optimal to use a singleton to avoid the overhead of all these jobs, but it is not as critical as before.
