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);
 }
 }
}

• Shelby115: Quick tip: The readonly keyword could be useful to help show intention of your member variables. For example, notifyCbr_ only gets assigned in the constructor and appears to be a good candidate for readonly. This way someone can come into your class and see that it isn't assigned anywhere but the constructor and more importantly it will be obvious that members without readonly are assigned in places other than the constructor.
• Ben Aaronson: Is there a reason you need this "Notify" mechanism, instead of just using a ConcurrentQueue?
• sfanjoy: Inbound objects are parsed and added individually. Consumption of the objects I want to grab in bulk and distribute on another thread. ConcurrentQueue will lock on dequeue of each object. I'm used to c++ STL where I can swap the contents of a vector for fast unloading of processed data. Notify mechanism provides ability to not have to poll or peek at the queue. Consuming thread is only bothered when data has arrived.
• sfanjoy: A refactored version and new questions here: codereview.stackexchange.com/questions/220197/…

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.

• sfanjoy: Alright! I have a lot to chew on now. I going to try to respond in order of the questions.
• Peter Taylor: I phrase a lot of feedback on this site in questions, but they're rhetorical. The goal isn't for you to give me an answer, but for you to ask yourself whether you have an answer which satisfies you. At the end of the day, I'm not approving or rejecting your commit. Don't feel any obligation to comment on the answer unless it's unclear.
• sfanjoy: Is there any reason not to use System.Action? - I have never used Action. After research...I do not see a use case to notify more than one observer so my class constructor can become NotifyVector(Action cbr). This worked in my tests. What is the procedure for presenting my refactored code?
• sfanjoy: Why the fully qualified names? / So why have the private constructor in the first place? - I learned to code on punch cards so I am used to explicitly stating what may be obvious. I did learn that default c# constructors are private. Not like c++.
• sfanjoy: This looks like an abuse of ref. - ref is used to quickly pass the objects to the consuming thread. I am used to STL and swapping the contents of a vector. Which brings me to the point of the why the class question. The goal is to prevent bottlenecks from network IO and efficient blocking to pass the data. The object(s) of interest can change while sitting in the vector between Notifications. The notification is only for a change in the collection object since the last time it was consumed.