Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 288041 - UIProcess should handle exception in runJob
Summary: UIProcess should handle exception in runJob
Status: RESOLVED FIXED
Alias: None
Product: Riena
Classification: RT
Component: Core (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P2 normal (vote)
Target Milestone: 1.2.0.M3   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-30 05:26 EDT by Ralf Ebert CLA
Modified: 2009-11-17 03:43 EST (History)
2 users (show)

See Also:

Attachments
Patch to enable default exception handling by Worker (1.16 KB, patch)
2009-10-26 04:51 EDT, Artur Schmidt CLA 		no flags Details | Diff
mylyn/context/zip (1.37 KB, application/octet-stream)
2009-10-26 04:51 EDT, Artur Schmidt CLA 		no flags Details
Patch to forward exceptions to the exception handler manager (1.45 KB, patch)
2009-11-06 08:43 EST, Artur Schmidt CLA 		christian.campo: iplog+
Details | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Ebert CLA 2009-08-30 05:26:35 EDT 
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 

If UIProcess.runJob throws an exception, it is almost ignored in UIProcess.InternalJob:179. It should be logged at least. I think an exception should not be even catched at this point, because this deactivates the default exception handling of the Eclipse Job Worker.

Reproducible: Always
Comment 1 Elias Volanakis CLA 2009-09-17 00:18:50 EDT 
Thanks Ralf.

From my POV we should at least log it - not ignore it.
Comment 2 Artur Schmidt CLA 2009-10-26 04:51:55 EDT 
Created attachment 150490 [details]
Patch to enable default exception handling by Worker

Exceptions in runJob() are no longer caught in UIProcess.InternalJob.run() to enable default exception handling by Worker.
Comment 3 Artur Schmidt CLA 2009-10-26 04:51:57 EDT 
Created attachment 150491 [details]
mylyn/context/zip
Comment 4 Elias Volanakis CLA 2009-10-26 14:12:33 EDT 
Hi Artur,

thanks for the patch. :-)

I agree with all changes, except for one thing: If I am not mistaken, runJob(monitor) is code that is typically overriden by the subclass (written by the app developer). So we cannot really know what could possibly go wrong there. In a case where runJob(...) throws an exception - what do we do? The run method is not going to return normally. So from my point of view we still need a catch clause, just to be on the defensive side of things:

try {
				success = runJob(monitor);
				// Exception handling is done by Worker
} catch(Throwable t) {
   // log it
   status = false;
}

Let me know what you think...

Thanks and kind regards,
Elias.
Comment 5 Artur Schmidt CLA 2009-11-06 03:43:28 EST 
Since UIProcess does not know how to handle exceptions from runJob(), in my opinion, it is reasonable to not handle the exception there (as Ralf suggested). The exception will propagate to Worker.run() where it will be handled/logged and the special case of OperationCanceledException will be taken care of. Otherwise, we would have to do the logging (and handling of OperationCanceledException) ourselves.
Comment 6 Christian Campo CLA 2009-11-06 03:51:45 EST 
The idea ist that UIProcess runJob method if finished with an exception must call the ExceptionHandler in Riena. There the application can handle the exception gracefully like log it, show a message box etc....

I believe thats what we should do to fix this bug.
Comment 7 Artur Schmidt CLA 2009-11-06 08:43:20 EST 
Created attachment 151553 [details]
Patch to forward exceptions to the exception handler manager

All exceptions from runJob (except for OperationCanceledException) will be handled by the exception handler manager
Comment 8 Christian Campo CLA 2009-11-06 09:17:22 EST 
patch committed. Bug fixed.

thanks Artur