C# asynchronous notification vector

Question

The purpose of this class is to efficiently notify another thread when data is available with minimal blocking to access/pass the data. The data reader/producer thread processes data in bulk which contains many objects. Instead of dispatching each object individually the objects are immediately or periodically pushed into a vector, it invokes an asynchronous notification and then continues to add objects to the vector. This allows the thread interested in the objects the ability to not have to poll for the objects and abstract itself away from the object creation. An example would be reading a continuous stream of data off the network where objects randomly arrive in different amounts and the objects get passed to another thread for processing. The goal is to minimize blocking and not miss a notification and leave an object in the vector. I know the .Net framework is vast so I may be reinventing the wheel. Have at it...

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Runtime.Remoting.Messaging;

namespace CoreObjects
{
  public class NotifyVector<T>
  {
    public delegate void NotifyCBR();
    private NotifyCBR notifyCbr_ = null;
    private System.Collections.Generic.List<T> vector_;
    private Object dataSyncObject_ = null;
    private volatile bool processing_ = false;
    private IAsyncResult ar_ = null;

    // Class Instantiation requires a Callback routine
    private NotifyVector() { }
    public NotifyVector(NotifyCBR cbr)
    {
      notifyCbr_ = cbr;
      vector_ = new System.Collections.Generic.List<T>();
      dataSyncObject_ = new System.Object();
    }

    // This member fuction invokes the callback routine
    public void Notify()
    {
      if (processing_ == false)
      {
        if (notifyCbr_ != null)
        {
          processing_ = true;
          ar_ = notifyCbr_.BeginInvoke(new AsyncCallback(this.EndNotify), null);
        }
      }
    }
    // Function used to implement the Async Notification/Callback
    void EndNotify(IAsyncResult ar)
    {
      // Retrieve the delegate.
      AsyncResult result = (AsyncResult)ar;
      NotifyCBR caller = (NotifyCBR)result.AsyncDelegate;

      // Call EndInvoke to complete/cleanup Async call 
      caller.EndInvoke(ar_);
      processing_ = false;
    }

    // Threadsafe add Object to vector
    public void AddObject(T obj)
    {
      lock (dataSyncObject_)
      {
        vector_.Add(obj);
      }
    }
    // Threadsafe pop Objects from vector
    public bool PopObjects(ref System.Collections.Generic.List<T> inlist)
    {
      bool retval = false;
      lock (dataSyncObject_)
      {
        if (vector_.Count() > 0)
        {
          inlist = vector_;
          vector_ = new System.Collections.Generic.List<T>();
          retval = true;
        }
      }
      return retval;
    }
  }
}

I use VS17 Community and my unit test is as follows:

using CoreObjects;

namespace UnitTests.CoreObjectTests
{
  public class Foo
  {
    private Foo() { }
    public Foo(string s) { data_ = s; }
    public string data_ { get; private set; }
  }

  [TestClass]
  public class NotifyVectorTests
  {
    public NotifyVector<Foo> collectedFoo_;
    private int fooCount_;
    public void CallbackRoutine()
    {
      System.Collections.Generic.List<Foo> fooList = null;
      while (collectedFoo_.PopObjects(ref fooList)) // vector handed over
      {
        for (int i = 0; i < fooList.Count; i++) // Do some work
          fooCount_ += 1;
        fooList.Clear();
      }
      fooList = null;
    }

    [TestMethod]
    public void ConstructAndNotify()
    {
      fooCount_ = 0;
      collectedFoo_ = new NotifyVector<Foo>(new 
        NotifyVector<Foo>.NotifyCBR(this.CallbackRoutine));
      collectedFoo_.AddObject(new Foo("One"));
      collectedFoo_.AddObject(new Foo("Two"));
      collectedFoo_.AddObject(new Foo("Three"));
      collectedFoo_.Notify(); // Performs the AsyncCallback
      Assert.AreEqual(fooCount_, 0);
      System.Threading.Thread.Sleep(1);
      Assert.AreEqual(fooCount_, 3);
    }
  }
}

Answer 1

I don't think that screen space is so limited that methods can't be separated by a blank line, and separation makes it slightly easier to see scope.

---

    public delegate void NotifyCBR();
    private NotifyCBR notifyCbr_ = null;

Is there any reason not to use System.Action?

---

    private System.Collections.Generic.List<T> vector_;

      dataSyncObject_ = new System.Object();

Why the fully qualified names?

On the basis of coding to the interface, I think vector_ should be typed as IList<T>.

---

    private Object dataSyncObject_ = null;

This is initialised to new System.Object() in the only constructor. You might as well inline it. As a matter of style, I believe it's generally preferred to use the keywords for core types:

private object dataSyncObject_ = new object();

---

    // Class Instantiation requires a Callback routine
    private NotifyVector() { }

So why have the private constructor in the first place? Is it left over from a refactor?

Also, why does the public constructor not check that its argument is non-null?

---

    // This member fuction invokes the callback routine
    public void Notify()
    {
      if (processing_ == false)
      {
        if (notifyCbr_ != null)
        {
          processing_ = true;
          ar_ = notifyCbr_.BeginInvoke(new AsyncCallback(this.EndNotify), null);
        }
      }
    }

I consider it bad style to compare to true or false. These nested ifs can be reduced to a single condition:

  if (!processing_ && notifyCbr_ != null)

processing_ may be volatile, but there's still a race condition. If you want to ensure that only one invocation is in progress at a time then you need to use some synchronisation technique: a separate lock, replacing it with a semaphore, ...

Is ar_ necessary? As far as I can see, you don't monitor progress. Remember YAGNI.

---

    // Function used to implement the Async Notification/Callback
    void EndNotify(IAsyncResult ar)
    {
      // Retrieve the delegate.
      AsyncResult result = (AsyncResult)ar;
      NotifyCBR caller = (NotifyCBR)result.AsyncDelegate;

      // Call EndInvoke to complete/cleanup Async call 
      caller.EndInvoke(ar_);
      processing_ = false;
    }

Since you're using VS17 I assume you're also using a recent version of C#. In that case, I'd move EndNotify into Notify to make its scope clear.

Why cast ar to AsyncResult?

With reference to my previous question about whether ar_ is necessary, you could replace it here with ar.

Again, the access to processing_ needs to be synchronised.

---

The Begin/End style of async has fallen out of favour. On the other hand, in this use case I'm not sure that there's a better way of doing it with async/await. Forcing asynchronous execution of a synchronous method is either longwinded or hacky. E.g. (WARNING: untested code)

    public void Notify()
    {
      lock (processingSyncObject_)
      {
        if (!processing_ && notifyCbr_ != null)
        {
          processing_ = true;
          Task.Run(async () =>
          {
            await Task.Delay(TimeSpan.FromTicks(1));
            notifyCbr_();
          }).ContinueWith(result =>
          {
            lock (processingSyncObject_)
            {
              processing_ = false;
            }
          });
        }
      }
    }

---

    // Threadsafe pop Objects from vector
    public bool PopObjects(ref System.Collections.Generic.List<T> inlist)
    {
      bool retval = false;
      lock (dataSyncObject_)
      {
        if (vector_.Count() > 0)
        {
          inlist = vector_;
          vector_ = new System.Collections.Generic.List<T>();
          retval = true;
        }
      }
      return retval;
    }
  }

This looks like an abuse of ref. The in value is unused, so it should be an out parameter.

Count() has a special case for lists, but I would still prefer Count for a variable typed as List<> or IList<>.

I find early returns more readable than single returns and flag Booleans.

---

I don't understand what the point of this class is. It has two completely separate functions: the notification and the list don't interact at all.