For the last 6 years or so at work, we have had an intermittent bug. In this case, intermittent means around once in 6 months or so. A little background to the problem first:
Our data access is done via what was originally Microsoft's SQLHelper class, passing in a stored procedure (and parameters), and our entities use the reader to load all their properties. Pretty straight forward stuff.
The problem:
Every few months, on the live system, a sproc will stop returning results, for no apparent reason. Calling the sproc from Sql Management Studio works fine. We have tried many different fixes: re-applying the sproc, calling the sproc from a different database login, re-pointing to the dev or test systems. None of it makes any difference, and then as suddenly as it stopped working, it starts working again.
A few days ago, I was attempting to track down some inconsistent search results, this time based around an fts index. Now this index is pretty large (at least, in my books it is) at around 1.5 million rows, and the column itself being a good few thousand words on average.
The code used for this problem boils down to the following
Sproc "ftsSearch":
Select id
from ftsTable
where contains(@query, searchColumn)
Reader Class:
public class FtsSearch : List<int>
{
public void Search(String input)
{
Clear();
var param = new SqlParameter("@query", SqlDbType.VarChar);
param.Value = input;
using (var reader = SqlHelper.ExecuteReader(DbConnection, "ftsSearch", param))
{
if (reader.HasRows)
{
while (reader.Read())
{
Add(reader.GetInt32(0));
}
}
}
}
}
Calling this function while the error is occurring, yields the following results:
Query: Results:
"Project Management" 20,000
"Project Manager" 15,000
"Project Manage*" 0
The first two queries are fine, the last however I would expect to bring back between 20,000 and ~35,000 results, and when we ran the sql from Management Studio, it brought back 29,000 results.
Now when debugging the function, we double checked everything was being called correctly - correct DB, correct sproc, correct login, correct (parsed) parameter.
Inspecting HasRows returns False. So we forced a call to Read() anyway, just to see what happened. An what do you know? Results, all there.
The reason that HasRows was returning false was that the sproc was triggering sql server to also send back a warning - in this case one about too many results (afraid I have lost the exact error code). Sadly this behaviour does not seem to be documented anywhere.
When writing a UserControl, it is often desired to expose one or more of the sub-controls design-time support to the user of your control. It is reasonably straight forward to do, and here is a rundown of how:
We start off with our UserControl, in this case the imaginatively named TestControl:
The code behind looks like this:
[Designer(typeof(TestControlDesigner))]
public partial class TestControl : UserControl
{
public TestControl()
{
InitializeComponent();
}
[DesignerSerializationVisibility(DesignerSerializationVisibility.Content)]
public ToolStrip ToolStrip
{
get { return tsMain; }
}
}
The first attribute on the class ([Designer(typeof(TestControlDesigner))]) just instructs that we want it to use our own custom designer file (which we create in a minute).
The next important point is the addition of the ToolStrip property, and the DesignerSerializationVisibility attribute that goes with it. This informs the winforms designer that any changes made to the ToolStrip should be stored in the hosting container's designer file. Without this attribute, no changes made in the designer would persist when you closed the designer.
Next, we add a reference to System.Design in the project, and create our TestControlDesigner class, inheriting from ControlDesigner:
public class TestControlDesigner : ControlDesigner
{
public override void Initialize(System.ComponentModel.IComponent component)
{
base.Initialize(component);
var control = (TestControl) component;
EnableDesignMode(control.ToolStrip, "ToolStrip");
}
}
As you can see, we have very little in here. The Initialize method is overriden, and we call EnableDesignMode on our ToolStrip (the property added to the TestControl earlier).
After compiling, we can go to our form (again, imaginatively named Form1), and add a couple of instances of TestControl to it from the tool box:
As you can see, the two control's ToolStrips contents is unique, and we have the ToolStrip's designer exposed in the forms designer.
When it comes to developing a new class, I don't tend to use TDD (Test Driven Development), I favour something I have named TAD - Test Aided Development. In other words, while I am for Unit Testing in general, designing something via writing tests sometimes feels too clunky and slow. I always write classes and methods with testing very much in mind, but I do not generally write the tests until later on in the process. This post covers roughly how I wrote the EventDistributor, and what points of note there are along the way.
The first phase in designing it, was the use case:
events.RegisterFor<PersonSavedEvent>(OnPersonSaved);
events.Publish(new PersonSavedEvent());
events.UnRegisterFor<PersonSavedEvent>(OnPersonSaved);
private void OnPersonSaved(PersonSavedEvent e)
{
/* ... */
}
From this use case, we are able to tell that we will have 0 -> n events, and each event will have 0 -> n subscribers. This points to some kind of Dictionary based backing field:
public class EventDistributor
{
private readonly Dictionary<Type, List<Action<Object>>> _events;
public EventDistributor()
{
_events = new Dictionary<Type, List<Action<Object>>>();
}
public void RegisterFor<TEvent>(Action<TEvent> handler)
{
}
public void UnRegisterFor<TEvent>(Action<TEvent> handler)
{
}
public void Publish<TEvent>(TEvent @event)
{
}
}
For populating the dictionary, we need to add an entry for a TEvent if there is not already one (and create a blank list of handlers), and append our new handler:
public void RegisterFor<TEvent>(Action<TEvent> handler)
{
var type = typeof(TEvent);
List<Action<Object>> handlers;
if (_events.TryGetValue(type, out handlers) == false)
{
handlers = new List<Action<Object>>();
_events[type] = handlers;
}
handlers.Add(handler);
}
This gives rise to the first problem: the line handlers.Add(handler); gives us a nice error of: Error Argument '1': cannot convert from 'System.Action<TEvent>' to 'System.Action<Object>'. To fix this, we need to create a new Action<Object> and inside that, cast the parameter to TEvent.
handlers.Add(o => handler((TEvent) o));
This does however make the UnRegisterFor method a little more tricky, as doing handlers.Remove(o => handler((TEvent)o)); doesn't work because they refer to different objects. Thankfully, as the Action's GetHashCode() gives the same result for each instance, providing the content is the same. We can use this to check for equality:
public void UnRegisterFor<TEvent>(Action<TEvent> handler)
{
var type = typeof(TEvent);
List<Action<Object>> handlers;
if (_events.TryGetValue(type, out handlers) == false)
{
return;
}
var hash = new Action<object>(o => handler((TEvent) o)).GetHashCode();
handlers.RemoveAll(h => h.GetHashCode() == hash);
}
The Publish method is nice and straight forward; if the event isn't registered, throw an exception, and raise each subscriber's handler.
public void Publish<TEvent>(TEvent @event)
{
var type = typeof(TEvent);
List<Action<Object>> handlers;
if (_events.TryGetValue(type, out handlers) == false)
{
throw new EventNotRegisteredException(type);
}
handlers.ForEach(h => h.Invoke(@event));
}
Now that we have a class roughly implemented, we create the first set of tests for it:
[Test]
public void When_publishing_an_event_without_a_handler()
{
var distributor = new Distributor();
Assert.DoesNotThrow(() => distributor.Publish(new PersonSavedEvent()));
}
[Test]
public void When_publishing_an_event_with_a_handler()
{
var wasCalled = false;
var distributor = new Distributor();
distributor.RegisterFor<TestEvent>(e => wasCalled = true);
distributor.Publish(new TestEvent());
Assert.IsTrue(wasCalled, "The target was not invoked.");
}
[Test]
public void When_publishing_an_event_and_un_registering()
{
var callCount = 0;
var increment = new Action<TestEvent>(e => callCount++);
var distributor = new Distributor();
distributor.RegisterFor<TestEvent>(increment);
distributor.Publish(new TestEvent());
distributor.UnRegisterFor<TestEvent>(increment);
distributor.Publish(new TestEvent());
Assert.AreEqual(1, callCount);
}
Other than the publish method is currently a blocking operation, there is one major floor to this class: it contains a possible memory leak. If a class forgets to UnRegisterFor a handler, the EventDistributor will still have a reference stored, preventing the calling class from being garbage collected. We can demonstrate this with a simple unit test:
[Test]
public void When_the_handling_class_does_not_call_unregister()
{
var count = 0;
var increment = new Action(() => count++);
var distributor = new Distributor();
using(var l = new Listener(distributor, increment))
{
distributor.Publish(new TestEvent());
}
GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
distributor.Publish(new TestEvent());
Assert.AreEqual(1, count, "OnPersonSaved should have only been called 1 time, was actually {0}", count);
}
public class Listener : IDisposable
{
private readonly Action _action;
public Listener(Distributor events, Action action)
{
_action = action;
events.RegisterFor<TestEvent>(OnTestEvent);
}
private void OnTestEvent(TestEvent e)
{
_action.Invoke();
}
public void Dispose()
{
}
}
While it would be simple to just say that it's the responsibility of the calling code to call UnRegisterFor, it would be better to handle that (likely) case ourselves. Good news is that .net has just the class needed for this built in: WeakReference. This class allows the target class to become disposed even while we still hold a reference to it. We can then act on the disposal, and remove our event registration.
Changing the Dispatcher to use this in its dictionary is fairly straight forward, and we even loose some of the casting needed to add items to the list:
public class Distributor
{
private readonly Dictionary<Type, List<WeakReference>> _events;
public Distributor()
{
_events = new Dictionary<Type, List<WeakReference>>();
}
public void RegisterFor<TEvent>(Action<TEvent> handler)
{
var type = typeof(TEvent);
List<WeakReference> recipients;
if (!_events.TryGetValue(type, out recipients))
{
recipients = new List<WeakReference>();
_events[type] = recipients;
}
recipients.Add(new WeakReference(handler));
}
public void UnRegisterFor<TEvent>(Action<TEvent> handler)
{
var type = typeof(TEvent);
List<WeakReference> recipients;
if (_events.TryGetValue(type, out recipients))
{
recipients.RemoveAll(o => o.Target.GetHashCode() == handler.GetHashCode());
}
}
public void Publish<TEvent>(TEvent @event)
{
var type = typeof(TEvent);
List<WeakReference> recipients;
if (!_events.TryGetValue(type, out recipients))
{
return;
}
recipients.RemoveAll(wr => wr.IsAlive == false);
recipients.ForEach(wr => ((Action<TEvent>)wr.Target).Invoke(@event));
}
}
The main points to note with this change is:
- We no longer need to create a new
Action<Object> just to cast the handler in RegisterFor.
UnRegisterFor no longer needs to create a new Action<Object> to get the hash code.Publish has an extra line to remove all handlers where the target has become disposed.
The next item to work on in this class is making the Publish method non-blocking, which can be done in a variety of ways.
The first option is to create a thread that will invoke all the handlers one after the other. This has the advantage of only one extra thread to deal with, but has the drawback of a single unresponsive handler will block all other handlers. Ignoring locking and cross-threading issues for the time being, it could be implemented like this:
public void PublishAsyncV1<TEvent>(TEvent @event)
{
var type = typeof(TEvent);
List<WeakReference> recipients;
if (!_events.TryGetValue(type, out recipients))
{
return;
}
var task = new Task(() =>
{
recipients.RemoveAll(wr => wr.IsAlive == false);
recipients.ForEach(wr => ((Action<TEvent>) wr.Target).Invoke(@event));
});
task.Start();
}
The second option is to have a separate thread/invocation for each handler. This has the advantage that each of the handlers can take as much time as needed, and will not block any other handlers from being raised, however if you have many handlers to be invoked, it could be slower to return than the first option. Again, ignoring locking and cross-threading issues, it could be implemented like so:
public void PublishAsyncV2<TEvent>(TEvent @event)
{
var type = typeof(TEvent);
List<WeakReference> recipients;
if (!_events.TryGetValue(type, out recipients))
{
return;
}
recipients.RemoveAll(wr => wr.IsAlive == false);
recipients.ForEach(wr =>
{
var handler = (Action<TEvent>)wr.Target;
handler.BeginInvoke(@event, handler.EndInvoke, null);
});
}
Personally, I go for the second method, as the number of handlers to be invoked is usually fairly small.
The next part to consider is what we conveniently ignored earlier - the cross-threading issues. The main issue we have is handlers being added or removed from the list while we are iterating over it.
Now I cannot remember where I read it, it was either from Jon Skeet, or from the Visual Basic .Net Threading Handbook, but the rough idea was "You should lock as smaller area of code as possible". This is to minimise the chance of a deadlock. Starting with the Publish methods, we only need to lock the parts that iterate over the list:
lock (Padlock)
{
recipients.RemoveAll(wr => wr.IsAlive == false);
recipients.ForEach(wr =>
{
var handler = (Action<TEvent>)wr.Target;
handler.BeginInvoke(@event, handler.EndInvoke, null);
});
}
The UnRegisterFor method is also very straight forward, as we again only need to worry about the iteration:
if (_events.TryGetValue(type, out recipients))
{
lock (Padlock)
{
recipients.RemoveAll(o => o.Target.GetHashCode() == handler.GetHashCode());
}
}
The RegisterFor method takes a little more locking than the other two, as this will handle the creation of the lists, as well as the addition to the list:
lock (Padlock)
{
if (!_events.TryGetValue(type, out recipients))
{
recipients = new List<WeakReference>();
_events[type] = recipients;
}
recipients.Add(new WeakReference(handler));
}
The full code listing and unit tests for this can be found here: EventDistributor Gist.
Table of Contents:
When working with MVP, it won't be long before you come across the need for multiple views on one form. There are several ways to achive this, and which you choose is really down to how you intend to (re)use your views.
The first method for dealing with the sub views is to expose them as a property of your main view, and set them up in the main view's presenter:
interface IMainView
{
ISubView1 View1 { get; }
ISubView2 View2 { get; }
/* Other properties/methods etc for MainView */
}
class MainView : Form, IMainView
{
public ISubView1 View1 { get { return this.subView1; } }
public ISubView2 View2 { get { return this.subView2; } }
}
class MainPresenter
{
private readonly IMainView _view;
private readonly SubPresenter1 _pres1;
private readonly SubPresenter2 _pres2;
public MainPresenter(IMainView view)
{
_view = view;
_pres1 = new SubPresenter1(view.View1);
_pres2 = new SubPresenter2(view.View2);
}
}
static class Program
{
static void Main()
{
using (var view = new MainView())
using (var presenter = new MainPresenter(view))
{
presenter.Display();
}
}
}
This method's advantage is simplicity, just create a new view and presenter, and call Display. The disadvantage is that the main presenter is tied to the sub presenters. A slight modification alleviates this:
interface IMainView
{
ISubView1 View1 { get; }
ISubView2 View2 { get; }
/* Other properties/methods etc for MainView */
}
class MainView : Form, IMainView
{
public ISubView1 View1 { get { return this.subView1; } }
public ISubView2 View2 { get { return this.subView2; } }
}
class MainPresenter
{
private readonly IMainView _view;
private readonly SubPresenter1 _pres1;
private readonly SubPresenter2 _pres2;
public MainPresenter(IMainView view, SubPresenter1 pres1, SubPresenter2 pres2)
{
_view = view;
_pres1 = pres1;
_pres2 = pres2;
}
}
static class Program
{
static void Main()
{
using (var view = new MainView())
using (var pres1 = new SubPresenter1(view.View1));
using (var pres2 = new SubPresenter2(view.View2));
using (var presenter = new MainPresenter(view, pres1, pres2))
{
presenter.Display();
}
}
}
The only change here is to pass our two sub presenters in to the main presenter as constructor parameters. Ultimately this seems to be the 'best' solution from a coupling point of view, however, if you are unlikely to change the sub presenters out for completely different sub presenters, then I would use the first method.
The final method for composing sub views is to push the responsibility to the actual main view, and make your main view pass any events and data to and from the sub view:
interface IMainView
{
String FirstName { get; set; }
String LastName { get; set; }
String AddressLine1 { get; set; }
String PostCode { get; set; }
/* Other properties/methods etc for MainView */
}
class MainView : Form, IMainView
{
private readonly SubPresenter1 _pres1;
private readonly SubPresenter2 _pres2;
void MainView()
{
InitializeComponent();
_pres1 = new SubPresenter1(subView1);
_pres2 = new SubPresenter2(subView2);
}
String FirstName
{
get { return subView1.FirstName; }
set {subView1.FirstName = value;}
}
String LastName
{
get { return subView1.LastName; }
set { subView1.LastName = value; }
}
String AddressLine1
{
get { return subView2.AddressLine1; }
set { subView2.AddressLine1 = value; }
}
String PostCode
{
get { return subView2.PostCode; }
set { subView2.PostCode = value; }
}
}
The disadvantage to this is that if one of the subViews were to change in anyway, the MainView also has to change to reflect this.
Out of the three methods outlined, Method 2 is my personal preference, especially when not using a DI Container, and Method 2 when I am using one. The 3rd Method I find is too brittle for most usage, especially during earlier stages of development when the UI is more likely to be changing.
