c# start async method within object constructor - bad practice?

Question

i have some code in an object constructor similar to

delegate DataSet MyInvoker;

public MyObject(Param1 p1)
{
    // property sets here
    // ...

    BeginMyAsyncMethod();
}

public void BeginMyAsyncMethod() 
{
    // set some properties
    // ...

    MyInvoker inv = new MyInvoker(SomeBeginMethod);
    inv.BeginInvoke(new AsyncCallback(SomeEndMethod), null);
}

My questions are:

1. Is this generally considered bad practice?
2. Would it be better (or good) practice to define a start method in my class which the user would call to carry out the async action?

This answer gives me the impression that leaving it to the user is bad practice although I am talking specifically about starting async methods in the constructor, not about the correct construction of an object.

Answer 1

This is easily accomplished with a slightly different approach. In all reality, this happens all the time, doesn't it? Here's a simple solution to give you an option without doing something dumb:

public class MyResource
{
    // do this instead of a constructor
    public async Task<MyResource> StartAsync()
    {
        await Task.Delay(1);
        return this;
    }
}

public class MyControl
{
    public MyResource Resource { get; set; }
    async void Button_Click(object s, EventArgs e)
    {
        // call start as if it is a constructor
        this.Resource = await new MyResource().StartAsync();
    }
}

Answer 2

It's typically never a good idea to do something in your constructor that is not directly related to creating the object instance. For example, if I didn't know the purpose of your MyObject class, I might not know that it spawns an asynchronous process when I create a new instance. That's generally a bad practice.

It almost sounds like you're saying; hey, create this object, and it's only going to be usable when this async process is finished; that's counter-intuitive.

If you want to do something like this, I would think you would be definitely served better by going to a factory pattern; you call a factory like so and it will create the object and call the method for you:

 var instance = MyObjectBuilder.CreateInstance();

 // Internally, this does
 //    var x = new MyObject();
 //    x.Initilizatize();
 //    return x;

If your object wont be usable until it's been finished initializing, then you should probably expose a property to check if it is ready, like so:

 instance.WaitForReady(); // blocking version
 if(instance.IsReady) // non blocking check

Answer 3

Yes because you're potentially introducing unexpected behaviors. Objects should be constructed and free from exceptions and unexpected behavior. If the async actions are still happening after the object is constructed and the user is trying to use it...

Also, what if the async method is still running and the user decides to destroy the object?

A good read: http://www.amazon.com/Framework-Design-Guidelines-Conventions-Libraries/dp/0321545613/ref=sr_1_1?s=books&ie=UTF8&qid=1314813265&sr=1-1

If your async method has to be async because it's a long running process then it needs to be moved to it's own method that gets called by the user, not when the object is instatiated.

You also have the potential issue of keeping a reference to the object if the user decides to destroy it before async method is done which means you could have a memory leak.

If this HAS to be done to initialize the object should be created using a factory or a builder.

Answer 4

It's definitely a bad idea to initiate anything in a constructor that might call back into the instance on another thread before the instance is fully constructed. Even if it's the last line of your constructor, the callback might be invoked before a subclass constructor finishes running since subclass constructor code runs after the base class constructor code. Even if your class is sealed today, it might be unsealed in the future, and tracking down this sort of problem if a subclass is added in the future would most likely be very expensive.

Basically, if you don't want to end up hunting heisenbugs, don't ever, ever do this sort of thing from a constructor. Instead, add a "start" method like you mention in your question #2.

Answer 5

I agree, bad idea. Add an Initialize method. You can also add a static method that returns the new object and executes the Initialize method. In this way you can also keep the constructor as private.

public class XXX
{
    private XXX()
    {
    }

    private void DoMyWeirdThingsAsynchronously()
    {
        ....
    }

    public static XXX PerformSomethingStrange()
    {
        XXX result = new XXX();
        result.DoMyWeirdThingsAsynchronously();
        return result;
    }
}

Answer 6

Personally I would avoid putting such code in a constructor. It would be better to provide a specific method for starting the asyncronous operation. Throwing exceptions from a constructor (other than validation exceptions) is a bad idea.