2
\$\begingroup\$

I'm implementing a Producer/Consumer but the generic interface doesn't require any knowledge of blocking collections or tasks.

public interface IReader
{
    IEnumerable<Data> Read();
}

Is it okay if my implementation of the IReader launches a task asynchronously and returns the consuming enumerable?

public IEnumerable<Data> IReader.Read()
{
    var t = Task.Factory.StartNew(() =>
    {
        BlockingCollection<Data> data = new BlockingCollection();
        DoRead(data);
        return data.GetConsumingEnumerable();
    });

    return t.Result;
}

My concerns are:

  1. Performance. Is there any negative effect from having the BlockingCollection on a different thread than the consumer?
  2. Is it bad practice to not indicate that the method is executing asynchronously?
\$\endgroup\$
  • \$\begingroup\$ Any benefit of your asynchrony here using Task.Factory.StartNew is immediately lost due to your use of BlockingCollection. You essentially spin up a new thread only to block whenever you're reading from it. I would advise you re-think your design. \$\endgroup\$ – Dan Pantry Dec 1 '15 at 8:55
5
\$\begingroup\$

Did you know that Task.Result blocks? Your code isn't asynchronous at all - and it can't be unless DoRead is.

Effectively your code is starting a new task and then blocking while it completes - there's no benefit to starting the task at all!

As a small aside, Task.Run is easier than using Task.Factory.StartNew.

If I were you, I'd just remove the Task.

\$\endgroup\$
  • \$\begingroup\$ D'oh! This was actually how I was trying to solve another problem I was having and I didn't realize I had killed the async behavior. Here is a new question: codereview.stackexchange.com/questions/112426/… \$\endgroup\$ – Kim Dec 1 '15 at 9:15
1
\$\begingroup\$

If you want producing and consuming to run in parallel, you need to run producing on one thread, and then consume from another thread. This means that DoRead() should be inside the Task, you have to enumerate GetConsumingEnumerable() before it returns. The code could look something like:

public IEnumerable<Data> IReader.Read()
{
    var data = new BlockingCollection<Data>();

    Task.Run(() => DoRead(data));

    return data.GetConsumingEnumerable();
}

This approach is not ideal if the producer could be faster than the consumer, because in that case, the blocking collection will keep growing. To avoid that, you can specify the capacity of the collection:

var data = new BlockingCollection<Data>(maxCapacity);
\$\endgroup\$
  • 1
    \$\begingroup\$ This is very much like my original code. The problem is if an exception occurs during the DoRead. Since there is no call to Wait or Result on the task the exception isn't thrown. When I tried to use ContinueWith to throw the exception it sill isn't being caught by the caller. \$\endgroup\$ – Kim Dec 2 '15 at 2:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.