Recently I’ve got another reminder why constructors should be simple. The situation in a nutshell: class BaseHandler subscribes to an observable in its constructor. The handler is virtual, class DerviedHandler adds incoming items ino a list initialized in its constructor. So, there is a potential virtual function call in constructor, but there is also a race condition. If there is nothing in the observable at the time of the construction, everything works. But if there is, virtual function gets called in the constructor, leading to the derived handler to operate on a non-initialized instance, and hilarity ensues.
Complete code is below, and is also on github. If the conditions are right, the exception occurs in DerivedHandler.HandleItem(), because _list is null. What is especially annoying, _list is marked readonly, so we mentally block the idea that it may, in fact, change value over time.
Yes, C++ has Resource Aquisition is Initialization paradigm, but C++ is a different language. C++ also does not allow virtual function calls in constructors: they blow up. C# does allow virtual function calls in constructors, which is probably fine, but combined with observables this leads to funny results.
The moral of the story is that we are all better off if constructors do not attempt to do anything fancy. Perform the bare minimum initialization, and then leave the rest to the Start() or Connect() method. Yes, you’ll have to call that method explicitly, but this is a small price to pay for not having to hunt ridiculous race conditions.
// Base class, provides virtual HandleItem() method called when new item is observed class BaseHandler : IDisposable { private readonly IDisposable _subscription; public BaseHandler(IObservable<int> items) { _subscription = items.Subscribe(HandleItem); } public void Dispose() { _subscription.Dispose(); } // default handler for observed item prints the value protected virtual void HandleItem(int n) { Console.WriteLine(n); } } // Derived class handles observed items by adding them to a list class DerivedHandler : BaseHandler { private readonly List<int> _items; // not initialized here on purpose public DerivedHandler(IObservable<int> items) : base(items) { _items = new List<int>(); } protected override void HandleItem(int n) { Console.WriteLine("Adding item: {0}", n); _items.Add(n); // this causes NullReferenceExceptino if called from the base's constructor } } class Program { static void Main() { using (var subject = new ReplaySubject<int>()) { // Publish an item before constructing any handler objects; this will cause an exception in constructor // Comment this out to prevent the exception subject.OnNext(42); using (new DerivedHandler(subject)) { // publish a new item to process subject.OnNext(43); Console.ReadLine(); } } } }
Permalink
Why even have virtual function at all?
Generally, instead of inheritance it is better to use composition. So base class would be just a “server” class that former “child” classes would call.
If former “child” classes need to pass custom behavior as a parameter into “server” class – then use delegate (Action/Func) field in “server” class.
Permalink
Yes, implementation inheritance went out of fashion some 20 years ago. Unfortunately, this news has not reached our company yet.
Permalink
Yeah, I try whenever possible only use constructors for dependency injection, period.
Permalink
When I suggested this, the response was “but then the object will be in an unusable state!” I disagreed, which led to a rather phylosophical argument and what constitutes “unusable state”.