Community
Participate
Working Groups
Created attachment 107731 [details] a simple application that demonstrates the problem Build ID: I20080617-2000 Steps To Reproduce: 1. Import the attached demo project 1. Start the application 2. Click on "Finish" in the wizard A StatusDialog should appear and block the UI before the wizard is closed. Instead, the dialog shortly appears and then the dialog and the wizard are closed automatically.
Actually if you return false from your performFinish method everything will work correctly. False means that finish request was refused. The status dialog does not block the thread and is usually displayed on the most top shell. So if you close your shell (f.e. wizard by specyfying true) the dialog will be closed, either. In the wizard you should use setErrorMessage to indicate that user made something wrong and override canFinish method, and this will cause finish button to be enabled/disabled. The real issue here is: StatusManager.handle(status, StatusManager.BLOCK) is not blocking. The javadoc of StatusManager.BLOCK says: A style indicating that the handling should block the calling method until the user has responded. This is generally done using a modal window such as a {@link Dialog}. So yes, it is a bug and I will try to fix it.
Reassigning to Krzysztof for investigation.
My thoughs: *. Blocking UI Thread causes the whole application to stop responding - any other status cannot be added until the user closes the dialog. *. More than one status can be displayed in the dialog, and each can be from different thread. *. The dialog can switch modality (reopening with different flag). However it is not an issue as long as dialog can become modal but cannot return to nonmodal state. Also we have to be ensure that a non blocking status arriving to the modal dialog will not block the parent thread. So: * we have to ensure that other threads will be able to communicate with UI one even if UI call is blocked. I wonder if it is possible at all. * alternatively we may consider acceepting fact that the dialog will not process any other status until user reacts to the UI blocking one. * we have to ensure that WorkbenchErrorHandler will remember which threads are blocked and which are not. * we can ignore modality switching becasue the dialog will become modal if and only if it receives blocking status adapter. Then no further modality change is possible. Further investigation still required. This bug is blocking 219136 because we cannot require any synchronic user action until we will have this bug solved. Thought? Comments?
(In reply to comment #3) I'm still a little confused because I can see in StatusDialogManager.addStatusAdapter() we handle modal, which we determined based on the style state BLOCK. > * we have to ensure that other threads will be able to communicate with UI one > even if UI call is blocked. I wonder if it is possible at all. Once the UI thread is blocked (e.g. with a modal dialog) then nobody else can communicate with it unless you specifically write code to poll from within the UI thread. > * alternatively we may consider acceepting fact that the dialog will not > process any other status until user reacts to the UI blocking one. We have two cases here: those from the UI thread and those from a background thread. The former case can't occur for the reasons above; the UI thread is busy and no other events can be processed (and status issues) until its done. In the latter case, does this not already work? If I have a status handler modal dialog up and someone in another thread keeps issuing statuses, they show up in the dialog right? > * we have to ensure that WorkbenchErrorHandler will remember which threads are > blocked and which are not. This is only true if you want to honour StatusManager.Block from non-UI threads. I'm not sure that was ever intended. The javadoc you cited below specifically calls out the use of a Dialog. The intention seems to be to prevent the user from proceeding while the being informed of the status, not of all processing to halt. Note it uses the word "should" not "must", which combined with it being a style flag, suggests its only a hint, not a contract. Thus I think we could only enforce it if its on the UI thread if we so chose. If you really wanted to block from any thread, I'm thinking a Display.syncExec() which then opens the dialog is enough (javadoc says, "The thread which calls this method is suspended until the runnable completes"). But again I'm not advocating this as I don't think it was the intention. > * we can ignore modality switching becasue the dialog will become modal if and > only if it receives blocking status adapter. Then no further modality change is > possible. Agree (until dialog is dismissed). As a final note, I'm wondering about WorkbenchStatusDialog.Manager.addStatusAdapter(), it has a comment midway down: // Add the error in the UI thread to ensure thread safety in the // dialog but I don't actually see anything that suggests the work is being done in the UI thread. I'd have expected to see a Display.syncExec() or asyncExec().
About final note: I will remove that comment. At the beginning of this method there is an assertion which checks if we are in UI thread. Because dialog.setBlockOnOpen(true) is not used, the dialog may appear modal but it will return from open() immediately. In consequence the whole routine ends pretty fast and the UI thread is never blocked. Even if you open the dialog from UI thread, other statuses will appear. > only a hint, not a contract. This is crucial. We can suggest that particular Status should be blocking or not, but one cannot assume that his hint will be respected, especially if error handler can be easily replaced. I see two possibilities here: 1. move the responsibility for blocking to the StatusManager 2. update javadoc to say clearly that we block users from proceedings, not threads, and that this is a hint. 3. wait ad 1. Seems to be difficult and brings us back to the discussion about UI/non-UI threads. Also, BLOCK is no longer hint, but is a contract, while other flags are hints. I do not like this inconsistency. ad 2. The simples approach, but it means that we resign from blocking threads. ad 3. Wait for a couple of months, maybe some new facts will appear or architecture will change.
I forgot - my preferred is 3.
(In reply to comment #5) > About final note: I will remove that comment. At the beginning of this method > there is an assertion which checks if we are in UI thread. Ah yes didn't spot that. First off, I notice that the javadoc doesn't state this as a requirement (that must be in UI thread). Second, would it not be better if we allowed from any thread then opened the dialog in a sync/asyncExec? This allows people to request that status' be processed regardless of the thread, which seems a good thing. So it seems that we can't actually handle statuses on any thread other than the UI thread, is this right? (ie. there isn't a syncExec() along the call chain that I missed?). > Because dialog.setBlockOnOpen(true) is not used, the dialog may appear modal > but it will return from open() immediately. In consequence the whole routine > ends pretty fast and the UI thread is never blocked. Even if you open the > dialog from UI thread, other statuses will appear. Good catch. This is the bug then isn't it? If BLOCK is used then we should dialog.setBlockOnOpen(true)? > > only a hint, not a contract. > This is crucial. We can suggest that particular Status should be blocking or > not, but one cannot assume that his hint will be respected, especially if error > handler can be easily replaced. Good point but I'd attack it the other way: if we believe its a contract, then those who provide their own handler must respect it too. This clearly increases the complexity of their job though (so another argument against). > I see two possibilities here: > 1. move the responsibility for blocking to the StatusManager > 2. update javadoc to say clearly that we block users from proceedings, not > threads, and that this is a hint. > 3. wait > > ad 1. Seems to be difficult and brings us back to the discussion about > UI/non-UI threads. Also, BLOCK is no longer hint, but is a contract, while > other flags are hints. I do not like this inconsistency. It can still be hint, we just observe it and try to do the right thing. Otherwise I'm not sure what BLOCK means (why have a hint if we never do anything based on?). But see next comment. > ad 2. The simples approach, but it means that we resign from blocking threads. First we need to decide what we think the current Javadoc implies. To me, its ambiguous. I suppose one can argue that if they used the BLOCK flag then in fact this is what they expected to happen, although I'd find it an odd programming practice on the part of the caller. Here's the two interpretations: Case A) Blocking dialog only if on UI thread We could open dialog blocking if both BLOCK and on the UI thread. Case B) Blocking UI dialog from any thread A combination of syncExec() and setBlockOnOpen() should work I'd think. And based on above discussion, if non UI thread and non-block then we do asyncExec(). Either way what we believe we're doing is *clarifying* the existing API. > ad 3. Wait for a couple of months, maybe some new facts will appear or > architecture will change. Fooling around with threading and blocking is tricky. I'd rather do this work sooner than later to give time for usage and feedback. I'm not sure what new information we'd be hoping for in the few months.
> * ie. there isn't a syncExec() along the call chain that I missed? Yes, WorkbenchErrorHandler was responsible for executing addStatusAdapter in UI Thread. Something is getting shape, step by step: For me BLOCK meant that the user should respond to the status before RCP will take any further action. Modal dialog did the thing, because usually reporting an error is the last thing you do. Florian has shown the example when such approach does not work and point that BLOCK should really block the calling thread. To clarify the API we have to emphasize that BLOCK is only a suggestion and should be used with great care. I have done some initial work (I'll attach patch) and it looks like we have to block UI thread when we want to use block flag. It is so, because opening a window has to be done in UI thread, and dialog.open is the blocking method. This means that if any thread uses BLOCK, UI thread will be blocked, and initial thread either, via syncExec. I have to admit that I do not like this solution at all, because no other thread can report an error. Current situation with corrected javadoc is better than that. Tomorrow I will try to investigate other approach, with semaphores.
Created attachment 108860 [details] Proof of concept Changing BLOCK behavior invalidates tests.
Created attachment 108861 [details] mylyn/context/zip
Created attachment 108947 [details] Proof of concept Another approach - When UI thread is called with BLOCK we simply display call-blocking dialog. If another thread uses BLOCK, we make him 'wait' and 'notify' when the dialog is closed. This is not a patch, just an approach presentation. I'd like to investigate this direction further.
Created attachment 109067 [details] Patch preview (in the middle of work)
Created attachment 109068 [details] mylyn/context/zip
(In reply to comment #8) Good analysis I agree with your points. > I have to admit that I do not like this solution at all, because no other > thread can report an error. That's my concern too. It basically gets us back to the old support where the user gets pestered by a sequence of dialogs appearing and being dismissed (say a runaway process causing errors). Also the situation is ambiguous. Did the use of BLOCK mean they didn't want the user to be able to interact with the UI until the error was dealt with, or that the thread be suspended until the user takes some action? Both makes sense when issued from the UI thread but its hard for me to imagine the latter being the right choice from another thread (e.g. imagine a builder generating errors, what purpose does suspending the process for each one provide?). As I think more on this, I think the right answer is modal dialog when issued from UI thread and nothing when from another thread, treating it as a hint. If in the future we decide we want blocking from any thread then we can add. This bug is just about the UI thread case and perhaps we should just fix the obvious and leave the ambiguous.
Created attachment 109140 [details] Patch RC1 I am more than suprised because I have spot that even while blocking the UI thread, other threads are able to report errors. I suspect that this is caused by the dialog#open which runs its own event loop while called in a blocking mode. This leads to the interesting situation, in which UI thread is blocked (the calling one), but is still running(event loop) - almost like quantum physics :D. I am happy with current patch. I block every thread that used 'BLOCK' hint. BLOCK generally should be used with great care (see bug 232036).
Created attachment 109141 [details] mylyn/context/zip
CC'ing Kim. I discussed this briefly with her since she's really good at these UI + threads kinds of problems.
Created attachment 112300 [details] Patch Now syncExec is used only for blocking calls and the javadoc for BLOCK flag was updated. I am going to create tests right soon.
Created attachment 112315 [details] Test for latest patch This test is a totally h4x0r but it works... The first attachment is a manual test case.
Created attachment 112636 [details] Szymon's proposal v01
Created attachment 112683 [details] Szymon's idea v01
Paul, could you take a look at the Szymon's proposal v01 and comment? It is just a prototype, so don't worry that I made one method public.
(In reply to comment #18) Some remarks: - we should not change the contract that WorkbenchStatusDialogManager#addStatusAdapter is called from the UI thread - we should not change the behavior of WorkbenchStatusDialogManager#addStatusAdapter, 'modal' controlled only modality, now there is 'blocking', that blocks control threads if true (that's why Szymon's proposal v01 is not correct too)
Created attachment 112686 [details] Szymon's idea v02 StatusManager#BLOCK javadoc updated.
(In reply to comment #24) > Created an attachment (id=112686) [details] > Szymon's idea v02 > For the case where you are not on the display thread in org.eclipse.ui.statushandlers.WorkbenchErrorHandler.handle(StatusAdapter, int), wouldn't you do similar code to what was in the UI thread: Display.getDefault().asyncExec(new Runnable() { public void run() { if (!PlatformUI.isWorkbenchRunning()) { // we are shutting down, so just log WorkbenchPlugin.log(statusAdapter.getStatus()); return; } getStatusDialogManager().addStatusAdapter(statusAdapter, modal); if (modal) { Display display = Display.getCurrent(); while (getStatusDialogManager().getShell() != null && !getStatusDialogManager().getShell().isDisposed()) { if (!display.readAndDispatch()) { display.sleep(); } } } } }); Another possibility if you don't want to collapse the if(!modal) to within the asyncExec(*) is to use a syncExec(*) for the modal case and put the "if (!display.readAndDispatch())" loop in the body of the syncExec(*). Other comments: 1) even as written, shouldn't the modal async have "if (!PlatformUI.isWorkbenchRunning()) {" just like the non-modal one? 2) If you are doing a while "// do nothing but wait" loop, aren't you supposed to have a call that will yield? Thread.sleep() might be too heavy weight, but something like that ... maybe Thread.yield() is appropriate? PW
Created attachment 112890 [details] Szymon's proposal v02 I added Paul's suggestions + code is polished.
Created attachment 112891 [details] Szymon's proposal v02
Bug 247818 needs to be fixed as soon as this patch is released.
Released to HEAD.
The getShell() call in the patch is a concern: + private void showStatusAdapter(StatusAdapter statusAdapter, boolean block) { ... + Display display = getStatusDialogManager().getShell().getDisplay(); because getShell() can return null if the dialog is null. In practice I'm not sure under which conditions this could be true; either the dialog had failed to open, wasn't open yet, or perhaps it could be headless status management. The while() just after that call checks getShell() != null so we could instead just move the getDisplay() down into the while.
Moved the getDisplay into the while where we know shell is not null
(Released to HEAD)
