Is it problematic to start a Task in the constructor?

Question

In the Call asynchronous method in constructor? question is no answer that, starts the async Operation in the constructor and store the Task in a member and an awaits it before using the resource:

    public class DeviceAccess
    {
        private readonly Task<Container> containerTask;
        public DeviceAccess(Database database)
        {
            containerTask = GetContainer(database);
        }

        private async Task<Container> GetContainer(Database database)
        {
            var conatinerResponse = await database.CreateContainerIfNotExistsAsync("Device");
            return conatinerResponse.Container;
        }

        public async Task<Device> GetDevice(string deviceId)
        {
            var container = await containerTask;
            return await doSomething(container);
        }
    }

In my case every Operation needs the resource, so I see no advantage to use some lazy loading.

Is it valid to start a async Operation in a constructor or can result this into problems?

What if the task throws exception? The DeviceAccess instance will still be constructed but GetDevice will always throw exception. I'm not sure if this is the expected behavior of your class but I think a class should throw an exception in their constructor if it can't retrieve the data they need to work correctly. — Leisen Chang, Oct 16 '19 at 08:53
As an extension of what @LeisenChang says there: that's one of the compelling advantages of an `OpenAsync` method as discussed in my answer below (or `ConnectAsync`, or `InitializeAsync` - the name doesn't matter); when *that step fails*, it will be *obvious* as to when, where and why it failed, because your stack-trace will be in the middle of an `await obj.OpenAsync(...);` — Marc Gravell, Oct 16 '19 at 08:57

score 4 · Accepted Answer · answered Oct 16 '19 at 08:40

4

The biggest problem I can see here is that [Value]Task[<T>] is an API that enables async, not a promise to be async; just because CreateContainerIfNotExistsAsync is named *Async and returns Task<T> - that doesn't actually mean it is async - it could run synchronously and return a result via Task.FromResult (aka "async over sync"). If you're not concerned about that problem, then fine I guess. But I wonder whether an OpenAsync() method that you call after construction would be more appropriate, i.e.

public class DeviceAccess
{
    private Container _container;
    public DeviceAccess() {}

    public async ValueTask OpenAsync(Database database) {
        if (_container == null)
            _container = await GetContainerAsync(database);
    }

    public async Task<Device> GetDeviceAsync(string deviceId)
    {
        var container = _container ?? throw new InvalidOperationException("not open");
        return await doSomething(container); // might be able to inline the "await" here
    }
}

answered Oct 16 '19 at 08:40

Marc Gravell

927,783
236
2,422
2,784

1

Though this might open a thread safety issue i guess with multiple calls to open? but then again... why would someone do that. – TheGeneral Oct 16 '19 at 08:48
2

@TheGeneral well, yeah, but that's generally true of *any* type that isn't explicitly designed to be thread-safe against concurrent callers; the usual boilerplate footnote of (to quote from MSDN/docs) "Public static (Shared in Visual Basic) members of this type are thread safe. Any instance members are not guaranteed to be thread safe." - ultimately, calling `OpenAsync` concurrently would be known as "using the API incorrectly", at which point you're in "undefined behavior" territory :) – Marc Gravell Oct 16 '19 at 08:51
@MarcGravell you mean that the code runs synchronously and does many things and then the constructor needs unexpected long? – sschoof Oct 16 '19 at 10:14
The new cosmos sdk has abandon the `OpenAsync` of v2 and do everything Lazy: https://github.com/Azure/azure-cosmos-dotnet-v3/issues/6 – sschoof Oct 16 '19 at 10:17
I agree with @TheGeneral in this one. Thread-unsafe means that you get undefined behavior when you access the resource from multiple threads. When you access the resource multiple times from the same thread, the behavior should be well defined IMHO (probably an `InvalidOperationException` in this case). – Theodor Zoulias Oct 16 '19 at 14:09
@TheodorZoulias when you add async, you need to stop thinking about threads and start thinking about execution paths; what would have been a single threaded `obj.Foo()` when dealing with non-async is only validly comparable to `await obj.FooAsync()` when talking async; if you do `var pending = obj.FooAsync(); obj.Blah(); await pending;` then *you've violated that*, and all blame is on you, the caller. – Marc Gravell Oct 16 '19 at 14:34
You have a point. Although, do you have any example of a built-in class that behaves this way? I mean a class that exposes an async method or property, and advertises itself as non-thread-safe in the documentation. I can't think of any myself. – Theodor Zoulias Oct 16 '19 at 15:11
@TheodorZoulias DbConnection ? edit: checked the docs, they don't make any explicit statement about thread safety, but: it isn't thread safe in this or virtually any other scenario – Marc Gravell Oct 16 '19 at 15:21
Good example! The documentation of [DbConnection.OpenAsync](https://docs.microsoft.com/en-us/dotnet/api/system.data.common.dbconnection.openasync) says: *Do not invoke other methods and properties of the DbConnection object until the returned Task is complete.* For this reason I would consider this behavior to be an exception and not the rule. – Theodor Zoulias Oct 16 '19 at 15:42
@TheodorZoulias I would consider it to be the rule; just because they've called it out doesn't make it otherwise - they're doing that *because the rule is important*, IMO (and: especially important in this case) – Marc Gravell Oct 17 '19 at 07:15
By "rule" I mean that most built-in classes that expose async members are thread safe. And most classes that don't, aren't. Classes like `DbConnection` that combine asynchrony and thread-unsafeness are not that common. And because of that, Microsoft's technical writers feel obliged to add these warnings in the documentation. – Theodor Zoulias Oct 17 '19 at 08:54
@TheodorZoulias "By "rule" I mean that most built-in classes that expose async members are thread safe" [citation needed] – Marc Gravell Oct 17 '19 at 09:06
I have not counted them one by one, but I would be willing to accept a pretty big bet that at least 75% of all async built-in classes are thread-safe, and at least 75% of all non-async built-in classes are not! – Theodor Zoulias Oct 17 '19 at 12:06
@TheodorZoulias are you counting static methods perhaps? if so: you shouldn't - static is different; I genuinely *can not think of a single case* where what you're saying is the case, except for APIs that **explicitly target concurrency** (such as the task scheduler API, or libraries like SE.Redis); some examples? any? counter examples: stream, sockets, pipelines, ado.net, ... – Marc Gravell Oct 17 '19 at 12:10
For example most classes in the namespaces `System.Collections.Concurrent`, `System.Threading`, `System.Threading.Tasks`, `System.Threading.Tasks.Dataflow` expose async methods/properties (or are awaitable), and are thread-safe. These are a lot of classes! – Theodor Zoulias Oct 17 '19 at 12:17
@TheodorZoulias System.Collections.Concurrent and System.Threading.Tasks are *explicitly targeting concurrency*; those *are the exceptions* (heck System.Threading.Tasks *is the implementation*) – Marc Gravell Oct 17 '19 at 12:37
I just checked out the source code of a `Stream` implementation ([`FileStream`](https://referencesource.microsoft.com/mscorlib/system/io/filestream.cs.html)), and I became really confused about what I am allowed to do with this class. Can I call `ReadAsync` (or `WriteAsync`), and then without awaiting the async method start looping with `Read` or `Write`? I don't know. The [documentation](https://docs.microsoft.com/en-us/dotnet/api/system.io.filestream) says nothing about that. – Theodor Zoulias Oct 17 '19 at 13:32
I retract my willingness to take that bet. Things are more complicated than I thought. :-) – Theodor Zoulias Oct 17 '19 at 13:38
1

I converted to use Lazy and so my constructor has now only resource allocation. But my class is still always usable and I can have no bug from a missing init calls. – sschoof Oct 17 '19 at 14:44

Is it problematic to start a Task in the constructor?

1 Answers1