Comment 1 Tod Creasey 2004-06-10 10:34:12 EDT

The problem is in the commands actionHandler

 /**
     * @see IHandler#execute(Map)
     */
    public Object execute(Map parameterValuesByName) throws ExecutionException 
{
        if ((action.getStyle() == IAction.AS_CHECK_BOX)
                || (action.getStyle() == IAction.AS_RADIO_BUTTON))
                action.setChecked(!action.isChecked());
        try {
            action.runWithEvent(new Event());
        } catch (Exception e) {
            throw new ExecutionException(
                    "While executing the action, an exception occurred", e); //
$NON-NLS-1$
        }
        return null;
    }
As this catches all of them it will also catch cancellations

Comment 2 Douglas Pollock 2004-06-10 10:36:39 EDT

I am happy to let OperationCancelledExceptions to percolate up from here, but I 
have one concern.  John, you say that this is a recent change, but this code 
hasn't changed in quite a while.  Has something changed in the underlying jobs 
code that would make this happen? 

Comment 3 John Arthorne 2004-06-10 12:24:06 EDT

It was my hunch that this changed recently, but looking at the code, I agree it
doesn't look like the commands have changed recently. It turns out that this
particular cancelation was a regression (see bug 66437), but it is still
possible for it to happen after the JDT fix.

I suspect this has not been seen because the large majority of commands will run
long running operations in the JFace ModalContext, which converts
OperationCanceledException into the checked exception InterruptedException,
which must then be caught and handled explicitly by the action. One could argue
that action handlers should not be throwing OperationCanceledException, but as a
last line of defence I still thing it should be handled by the command
infrastructure (perhaps just logged but not with an error dialog to the user?)
CCing Nick for a second (or third) opinion on how and by whom cancelation should
be handled.

I have also logged another bug against JDT for this (bug 66541), since the stack
illustrates that the open type command is doing long running work in the UI
thread without some form of runnable context (progress dialog, etc).

Comment 4 Nick Edgar 2004-06-10 13:16:07 EDT

Neither the JFace action framework nor the commands mechanism have ever had
special handling for OperationCanceledException.  This needs to be handled by
the action itself (JDT's OpenTypeAction).  If it's not aware of the possibility
of this exception, then it needs to handled lower down in its call chain, e.g.
by whoever calls IWorskpace.run, which is spec'ed to throw
OperationCanceledException.

Comment 5 Dirk Baeumer 2004-06-10 13:37:33 EDT

The operation canceled excpetion is thrown by a method that doesn't take a 
progess monitor. So far we only expected operation cancel exceptions or 
interrupted exceptions in cases where we passed a progress monitor. Following 
Nick arguments clients now have to wrap all calls with a try/catch 
OperationCanceledException. Is this really something we want to do ?

CC Philippe, since the operation cancel exception is thrown by 
SearchEngine.createWorkspaceScope.

Comment 6 John Arthorne 2004-06-10 14:36:23 EDT

This was raised and discussed in detail in bug 57594, but I don't know how it
was eventually resolved.

Comment 7 Erich Gamma 2004-06-16 05:44:16 EDT

The dialog that shows up in this case to the user is scary:

The command for the you pressed failed
  Reason:
   The command for the key you pressed failed

Therefore we should add a band-aid for 3.0. Once the band-aid is in it should 
be tagged with 3.1.

Comment 8 John Arthorne 2004-06-16 11:54:28 EDT

Erich, are you suggesting a band-aid in the open type dialog, or in the platform
command support? Since the JFace ModalContext handles
OperationCanceledException, there is some precedent for handling it at the
platform level. In 3.0 OperationCanceledException will be more common than
before, since we now have a "default progress monitor" that opens a dialog
whenever any lock is blocked in the UI thread, and the user is always allowed to
cancel it, causing OperationCanceledExceptions that were not previously possible.

Comment 9 Erich Gamma 2004-06-16 12:11:19 EDT

Unfortunatly, it is a local band-aid only in the Open Type dialog action, 
i.e., the OperationCanceledException will be caught so that it doesn't 
propagate. 

I understand that this doesn't fully address the problem. This scenario 
illustrates the more general problem nicely:
1) the OpenTypeAction calls SearchEngine.createWorkspaceScope()
   -> the API doesn't take a ProgressMonitor
   -> the signature doesn't declare an exception
   -> the method returns a non-null search scope.
2) createWorkspaceScope calls a method which takes progress monitor
   as a side effect.
3) null gets passed
4) the UI is blocked and the user operation is canceled

An OperationCanceledException is fired and caught by the Command 
infrastructure.

Given the current API to create a workspace scope I don't see how this problem 
can be addressed at the platform level. Do you have ideas?

Comment 10 John Arthorne 2004-06-16 12:19:23 EDT

I was just suggesting that when OperationCanceledException is caught and handled
(in WorkbenchKeyboard.logException), it could silently log the failure but not
open the "scary" dialog for the user. Logging the exception (rather than
ignoring it) makes sense because it should not have reached that level and
should have been handled higher up in the call chain.

Comment 11 Erich Gamma 2004-06-16 14:34:32 EDT

Supressing the error dialog in the case of an OperationCanceledException in 
WorkbenchKeyboard is the better band-aid and I'm favor of doing this.

CC-ing Michael.

Comment 12 John Arthorne 2004-06-16 14:39:40 EDT

Created attachment 12306 [details]
Another cancelation reported as error in RC2

Comment 13 Dirk Baeumer 2004-06-16 17:24:53 EDT

Erich, the stack trace attached from John is an operation canceled exception 
thrown when the package explorer content provider fetches children from the 
Java model. The only hack I can think of for 3.0 is to catch this exception 
and return an empty array of children. The problem with this is that this 
project will never have children then unless the user open/closes the project 
or the package explorer. Any opinion ?

IMO we need a better story for this post 3.0 since putting "random" band-aids 
into your code doesn't seem to be the right solution. As createWorkbenchScope 
getPackageFragmentRoots doesn't take a progess monitor.

Comment 14 Nick Edgar 2004-06-16 17:31:22 EDT

I find it strange that OperationCanceledException is thrown at all by the
infrastructure.  Previously, this would only be thrown by the operation itself,
after explicitly checking:
 	if (monitor.isCanceled())
		throw new OperationCanceledException();

So even if it ran the operation in a progress dialog (or other runnable context)
that allowed cancel, and the user hit cancel, OperationCanceledException would
only be thrown if the operation checked monitor.isCanceled() and decided to
throw it.

Comment 15 Dirk Baeumer 2004-06-16 17:43:09 EDT

We discussed this and as far as I can remember the problem is as follows:

- blocking dialog is up
- user presses the cancel button
- since the actual thread/operation to be canceled is blocked it can't
  check isCanceled(), so the infra structure decides to throw the exception
  "on behalf" of the operation.

But I agree that this is strange and we should definitelly come up with a 
better solution here. One idea would be that the caller of run(Operation...) 
has to provide a cancel handler which gets control when the user presses the 
cancel button.

Comment 16 Dirk Baeumer 2004-06-16 17:44:10 EDT

Created attachment 12337 [details]
Patch for the OpenTypeAction.

Comment 17 John Arthorne 2004-06-16 18:04:57 EDT

Re comment #14 - the API being called in both of these cases is IWorkspace.run.
This method takes a progress monitor and can throw OperationCanceledException.
The implementation of IWorkspace.run will throw OperationCanceledException
when all of the following conditions are true:

1) An operation is running in the UI thread
2) The operation has become blocked by activity in another thread.
3) The progress monitor is null.
4) The user has clicked cancel in the progress dialog that we open in this
situation.

This is new in 3.0 (since M4) - the old behavior was lengthy hangs with no
painting of the UI. Most of these cases end up being involved with classpath
computation, which presumably only began to perform locking operations with the
introduction of classpath containers. The longer term solution is to avoid long
locking operations when in the UI thread, or within API calls that have no
progress monitor parameter. This has been discussed at length in bug 57594
and bug 57181. I was only suggesting a workaround to handle this situation a bit
better - I know it is not the right answer.

Comment 18 Dirk Baeumer 2004-06-17 05:01:49 EDT

Fix released for OpenTypeAction. Reviewed by Martin Aeschlimann.

Comment 19 Dirk Baeumer 2004-06-17 08:35:44 EDT

The operation canceled exception thrown when the user expnads a node in the 
tree has to be handled by JFace. The only change JDT/UI could do is to catch 
the exception in the content provider and then returning an empty array. But 
this would mean that the "node" is dead. Meaning although it has children it 
can never be expanded again.

JFace could detect this case in expand node and could still keep the dummy 
child element so that the node can be expanded again.

Moving to Platform/UI for consideration.

Comment 20 Michael Van Meekeren 2004-06-17 13:46:09 EDT

doug please investigate what we can do here regarding comment #19

Comment 21 Douglas Pollock 2004-06-17 15:08:00 EDT

I believe Dirk is referring to IStructuredContentProvider.getElements().  
Unfortunately, this method is called by more than just 
StructuredViewer.getRawChildren().  It is called by the following methods: 
+ StructuredViewer.getRawChildren(Object) 
+ CheckedTreeSelectionDialog.createSelectionButtons(Composite) 
+ CheckedTreeSelectionDialog.evaluateIfTreeEmpty(Object) 
+ ElementTreeSelectionDialog.evaluateIfTreeEmpty(Object) 
+ ListSelectionDialog.okPressed() 
+ ResourceTreeAndListGroup.aboutToOpen() (and several other methods) 
+ CheckboxTreeAndListGroup.aboutToOpen() (and several other methods) 
+ NewProgressViewer.contentProviderGetRoots(Object) 
+ ActionSelectAll.run() 
 
This is a lot of places to try to defend against OperationCanceledExceptions, 
when (ultimately) it is the structured content provider that knows whether this 
kind of exception can be thrown. 
 
I think the "right" solution, might be to create an 
IStructuredContentProvider2, which allows for a concept of cancellation. 
 
Deferring the remainder of this fix from RC3.  After 3.0, we will consider this 
fix.  Changing to the "Viewers" component, and moving back to Tod (current 
owner of viewers). 

Comment 22 Dirk Baeumer 2004-06-17 20:12:43 EDT

Douglas,

wouldn't it be enought to fix this bug to handle the operation canceled 
exception in AbstractTreeViewer.createChildren ?

Comment 23 Douglas Pollock 2004-06-17 20:48:46 EDT

From my perspective, that would be fixing one particular place where things 
could go wrong, while leaving the others broken.  This seems wrong.  Unless you 
think that IStructuredContentViewers used in those other cases will never use 
the jobs infrastruction...? 
 
But then again, this in't my area of expertise, so I might be missing something 
here. 

Comment 24 John Arthorne 2004-06-18 10:05:18 EDT

It seems like a losing battle to handle cancelation in the viewer framework. I
would expect it to just percolate up to the event loop and be handled by the
workbench advisor's exception handler - allowing the app to decide how they want
to handle cancelation.

Comment 25 Nick Edgar 2004-06-23 13:07:16 EDT

It would be possible to make viewers be aware of cancelation, either via new API
or having it handle OperationCanceledException (although we generally try to
minimize the dependencies on runtime from JFace).
Just having it percolate up to the event loop may leave the viewer in an
inconsistent state.  For example, in AbstractTreeViewer.handleTreeExpand, it
calls createChildren.  This deletes any dummy element (needed to show the +),
then asks for the child elements from the content provider, then creates the
corresponding SWT items.  If the call to the content provider fails, either due
to an OperationCanceledException or some other exception, then the parent node
will be expanded but will have no children.  Fortunately, looking at the way
createChildren is implemented, it should ask the content provider again if the
parent node is collapsed and then expanded again.

Comment 26 Tod Creasey 2004-07-05 16:24:13 EDT

We (Nick, John, Doug and myself) have discussed this and have come to the 
consensus that allowing an OperationCancelledException through is a 
programming error as Nick says.

However we think that it would be best that the commands support uses the 
exception handling from the WorkbenchAdvisor rather than its own.

Forwarding to Doug for that work.

Comment 27 Dirk Baeumer 2004-07-06 06:19:54 EDT

Can you please advise what a content provider should do in this situation to 
avoid that the viewer ends up in a dead state. 

Comment 28 John Arthorne 2004-07-06 10:29:53 EDT

The root of the problem in these JDT cases is the classpath container
initialization that locks the workspace when calling seemingly read-only Java
model methods. This is unexpected from the point of view of a UI-based caller,
so they do not fork the call into a background thread. and can easily result in
the UI thread being locked unexpectedly. In another bug I suggested the
possibility of JDT core persisting the value of classpath containers between
sessions, removing the need to perform this complex container initialization at
the start of every session. Then JDT UI could then safely call these methods
without fear of locking or cancelation.

From the JFace viewer point of view, OperationCanceledException is just another
runtime exception. As long as viewers are able to recover gracefully from
runtime exceptions (as comment #25 suggests), then I don't see what else they
need to do.

Comment 29 Dirk Baeumer 2004-07-06 12:16:38 EDT

Comment #26 gives the impression that Platform/UI decided to not handle the 
case of a canceled getChildren call in the viewers. If this isn't the case 
then skip the rest of this comment.

IMO we have the following "general" situation:

- methods can throw an OperationCancelException. This is especially true if
  the method takes a progress monitor.

- if a content provider calls such a method it must deal with the operation
  cancel exception

- due to the current implementation of the Tree Viewer there is nothing the
  content provider can do to avoid that the node to be expanded gets dead.

So we need some additional API to signal such a situation to the Viewer. I see 
two possibilities:

- the content provider API takes a progress monitor and is allowed to throw
  an OperationCancelException or an InterruptedException.

- we get some additional API on a viewer to inform it about the cancelation
  of a getChildren/getElement call

Even if JDT/Core would fork the call into the background then the only 
difference would be the type of the exception. We would get an Interrupted 
exception not a OperationCancelException. But the content provider would still 
need to signal the cancelation to the viewer.

Comment 30 Nick Edgar 2004-07-06 14:59:43 EDT

Without introducing new API, it may be possible to force the viewer to "fix"
itself after a cancelation, e.g. using setExpanded and/or refresh(Object).  This
may need to be done outside the expand callback.  Note that refresh will drop
the children if the parent node is collapsed, causing it to request the children
again the next time it's expanded.

However, I think we really need to step back and think about how to really make
the viewer framework able to handle long running operations and also
cancelation, and simplify this for the programmer.

Comment 31 Douglas Pollock 2004-07-14 18:18:52 EDT

At the very least, the key binding architecture shouldn't handle the exception 
itself.  It should pass it through the WorkbenchAdvisor.  I'll do that piece of 
the work to start with. 
 
It might be appropriate to open a separate bug to deal with the 
OperationCanceledException in general.  I'm not sure, but this seems like a 
design flaw.  RuntimeExceptions can interrupt the flow of execution (leaving 
partially initialized data).  So, doesn't this mean that any place we call out 
to third-party plug-ins (from UI code) we need to defend against 
RuntimeExceptions?  I mean, this was always the case, but the 
OperationCanceledException seems like it might make this a little easier to 
trigger. 
 

Comment 32 Douglas Pollock 2005-04-21 16:30:13 EDT

I've decided not to make any changes to the command architecture regarding how
it deals with exceptions.  I had a previous request to ensure that a dialog
opens if a command fails (Bug 52138), and I still believe that this is the right
thing to do.  Unfortunately, the WorkbenchAdvisor does not currently do this,
nor do I feel that it is necessarily the right thing for the WorkbenchAdvisor to do.

There might be something that can be done by the JFace framework to better
handle OperationCanceledExceptions.  In particular, there is a sub-thread in
this bug talking about viewers.  I would recommend that, if this is still
important, a separate bug be opened.

Comment 33 Dirk Baeumer 2005-04-22 04:43:27 EDT

Open bug 92347 to track the problem with the stale viewer in the present of
runtime exceptions.