Wednesday, November 21, 2007

Always use EndInvoke for each BeginInvoke you call

Let's say you have the delegate:
delegate void Foo(string x);

And a method that does some job and then it might crash. (here crashes always).
void Method(string txt)
{
//do code here;
throw new Exception("Unhandled exception never caught unless a debugger attached");
}

And another method that starts the first one asynchronous on a thread pool thread.
void CallingMethodAsynch()
{
Foo delegFoo=this.Method;
delegFoo.BeginInvoke("text",null,null);
}

The most common mistake made by programmer lazyness is passing null,null as parameters on BeginInvoke.
This brings strange scenarios into your multithreaded application by never discovering some exceptions; the thread will be aborted and you will never be noticed inside your app unless you do some advanced coding or simply use the asynch pattern.

The usual pattern would be to add the method:

void CallBackMethod(IAsynchResult aRes)
{
if(aRes==null OR aRes.AsynchState==null)
return;
Foo deleg=aRes.AsynchState as Foo;
if(Foo==null)
return;
Foo.EndInvoke(aRes);
}

and replace BeginInvoke("text", null, null) with BeginInvoke("text", CallBackMethod, foo);

This means that in case an unhandled exception happened on the sepparate thread, you will trap the error on EndInvoke - either put a catch or let it propagate.

I encourage to avoid catching errors in multithreaded apps, let the error propagate, crash the application, analyze the crash and improve your code, rather than catching errors and have a bad performance on your app.

A few usefull links(some oldie but good):
http://msdn2.microsoft.com/en-us/library/ms810439.aspx
http://weblogs.asp.net/justin_rogers/articles/126345.aspx

5 comments:

Firas Haj-Husein said...

Thanks for the excellent note.

By the way, you don’t need to pass the delegate to BeginInvoke, since the object passed to the callback method is actually an AsyncResult instance, which carries the delegate in its AsyncDelegate property.

Leedrick said...

But what if you want to fire and forget? Won't this method cause the parent thread to wait for the worker thread to complete?

Cosmin S said...

Firas:
I got this habbit because it happened to me once that garbage collector was somehow doing some cleanup wiping the objects before callback and the callback was never fired. If I send it as asyncstate it's cheaper(cast wise) and safer(the GC sees the reference associated). That's what I believe, feel free to tell me other solution.

Leedrick: Usually when you fire and forget you don't have error control. Those threads will simply die or go orphan and you won't be able to see what the heck is going on with your application that is not doing what it is supposed to do. Memory wise is not as good either. My 2 cents... :)

Anonymous said...

Hello, below is my code, but Message box does not show up.
please give me some advice.

public delegate void AsyncDelegateMethod(string tmp);
public void Method(string tmp)
{
throw new Exception(tmp);
}
public void GetException(IAsyncResult isr)
{
try
{
AsyncDelegateMethod asm = (AsyncDelegateMethod)isr.AsyncState;
asm.EndInvoke(isr);
}
catch (Exception ex)
{
ShowMessage(this.Page, ex.Message.Trim());
}

}
protected static void ShowMessage(Page aPage, string aMessage)
{
ScriptManager.RegisterStartupScript(aPage, typeof(UpdatePanel), "aaa", "alert('" + aMessage + "')", false);
}

protected void Button1_Click(object sender, EventArgs e)
{
AsyncDelegateMethod adelt = new AsyncDelegateMethod(Method);
AsyncCallback acb = new AsyncCallback(GetException);
IAsyncResult iasr = adelt.BeginInvoke("123", acb, adelt);
}

Cosmin S said...

Hey,

From the register script tag I realize you want to do some ASP.Net async voodoo-magic. :)
Well, based on the ASP.Net lifecycle (do some research on that first) the request arrives from the client on your server, you handle it, then the response goes back, and that's it.
When you register a script you actually include some jscript in the response which gets executed when the response completes to be transfered from the server to the client browser.

You are trying here to split the threading on the arrival of the request serverside to do something else (in this case throw an exception).
But, the actual register script can't work because you are no longer on the thread that managed the request/response and the response is long well sent and discarded to the client. (e.g. the lifecycle ended and you don't have a way to push things to the client based on classic http model).

You can do some manual polling by rechecking with some Jscript/Ajax from the client browser, when the request is completed. (that is, when the asynch callback is called, store in some place the result of the action and then at some hearbeat intervals, you call from client with ajax/jscript other method from server that checks the result).

Hope that makes sense.