Bug 497175

Summary:	Exceptions thrown in a method that is invoked via Control.Invoke are not propagated to the caller
Product:	[Mono] Mono: Class Libraries	Reporter:	Tom Spink <tspink>
Component:	Windows.Forms	Assignee:	Mono Bugs <mono-bugs>
Status:	RESOLVED FIXED	QA Contact:	Mono Bugs <mono-bugs>
Severity:	Minor
Priority:	P5 - None	CC:	calberto.cortez, robertj
Version:	unspecified
Target Milestone:	---
Hardware:	Other
OS:	All
Whiteboard:
Found By:	Development	Services Priority:
Business Priority:		Blocker:	---
Marketing QA Status:	---	IT Deployment:	---
Attachments:	A patch to introduce exception propagation to Control.Invoke A better patch to introduce exception propagation to Control.Invoke

Description Tom Spink 2009-04-22 08:30:55 UTC

Description of Problem:
When Control.Invoke is used to invoke a method, if that method throws an exception it does not get propagated back to the caller - rather an unhandled exception occurs.  The Microsoft implementation does propagate this exception, which can then be caught with in a try..catch block.

To fix this, AsyncMethodResult.EndInvoke should throw the exception (if any was generated), and this will automatically fix Control.Invoke, and indeed any handlers in async callbacks using Control.EndInvoke.

I've created a patch to introduce this functionality, but am unsure as to where to post it for review.

Steps to reproduce the problem:
1. Create a method that throws an exception.
2. Call that method using Control.Invoke, and wrap it in a try..catch block


Actual Results:
An unhandled exception.

Expected Results:
A caught exception.

How often does this happen? 
Always.

Comment 1 Carlos Alberto Cortez 2009-04-27 16:20:37 UTC

Post here the patch, and I will review it. Thanks!

Comment 2 Tom Spink 2009-05-01 07:46:19 UTC

Created attachment 289405 [details]
A patch to introduce exception propagation to Control.Invoke

Comment 3 Robert Jordan 2009-08-07 19:57:39 UTC

The first "throw ex;" in the patch must be a "throw;"

Comment 4 Carlos Alberto Cortez 2009-08-08 03:56:03 UTC

Besides to the comment by Robert, I think that in:


@@ -513,7 +525,14 @@

                        try {
                                AsyncMethodResult result = data.Result;
-                               object ret = data.Method.DynamicInvoke (data.Args);
+                               object ret;
+                               try {
+                                       ret = data.Method.DynamicInvoke (data.Args);
+                               } catch (Exception ex) {
+                                       result.CompleteWithException (ex);
+                                       return;
+                               }
+                               
                                result.Complete (ret);

We could move the AsyncMethodResult line out of the try block, so we can use a catch block to call result.CompleteWithException, instead of having two try blocks - which I don't like very much.

Besides that, the patch is fine. 

Thanks!

Comment 5 Tom Spink 2009-08-09 10:46:40 UTC

Created attachment 311470 [details]
A better patch to introduce exception propagation to Control.Invoke

Okay, so I've updated the patch to introduce those changes - thanks for reviewing guys!

Comment 6 Carlos Alberto Cortez 2009-08-13 15:37:41 UTC

I applied your patch in rev 139853. Thanks!

Carlos.