4

For whatever reason, ThreadPool's QueueWorkItem doesn't return an IAsyncResult or some other handle to the work item, which would allow to wait until it's completed. There are RegisterWait... methods, but you have to pass a WaitHandle and creating them is expensive (see IAsyncResult documentation, which advises you to delay creating a WaitHandle until requested). The Task Parallel Library will fix this lack, but there is a long wait before that's available. So, are there any problems with this design:

public class Concurrent<T> {
    private ManualResetEvent _resetEvent;
    private T _result;

    public Concurrent(Func<T> f) {
        ThreadPool.QueueUserWorkItem(_ => {
                                         _result = f();
                                         if (_resetEvent != null)
                                             _resetEvent.Set();
                                     });
    }

    public WaitHandle WaitHandle {
        get {
            if (_resetEvent == null)
                _resetEvent = new ManualResetEvent(_result != null);
            return _resetEvent;
        }

    ...

EDIT: I asked a follow-up question about the concerns which arise when using async delegates instead of the ThreadPool.

Community
  • 1
  • 1
Alexey Romanov
  • 167,066
  • 35
  • 309
  • 487

2 Answers2

6

Well, you've got a race condition between fetching the WaitHandle and setting it. Do you really want the caller to be waiting forever if they happen to be a tiny bit late?

You should probably do some appropriate locking and keep an "I've finished" flag so that if you do create the WaitHandle after it's finished, you set it before returning it.

I'd also personally write a static factory method rather than just using a public constructor - or make it a "create and then explicitly start" pattern. Queuing the work item in the constructor feels weird to me.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Note that I pass `_result != null` as the parameter to the ManualResetEvent constructor. (This won't work for structs, but the idea is the same.) `_result != null` serves as the finishing flag here (not in the actual implementation, of course). Yes, judicious locking is needed here. (continued...) – Alexey Romanov Jan 02 '09 at 00:46
  • A static factory method goes without saying, I very rarely write a generic public constructor without a corresponding static method. – Alexey Romanov Jan 02 '09 at 00:49
  • I wouldn't make the constructor public in the first place though. You should also consider methods which return null... – Jon Skeet Jan 02 '09 at 08:07
  • As I said, `_result != null` won't be used in the actual implementation (this would also work badly if the method throws an exception). – Alexey Romanov Jan 02 '09 at 12:28
3

Why aren't you using an asynchronous delegate, as demostrated here:

http://msdn.microsoft.com/en-us/library/h80ttd5f.aspx

That would make Concurrent obsolete, no?

  • 1) Concurrent provides a more easily usable API (I think); 2) Concurrent is just one subtype of Future, there are others which work differently (lazy values, promises); 3) (Most important) Because I didn't notice that Concurrent could be implemented as just a facade over asynchronous delegates. – Alexey Romanov Jan 02 '09 at 02:09
  • Ah, I thought from the start this had something to do with futures :-). –  Jan 02 '09 at 10:13