Running with Code Like with scissors, only more dangerous

28Mar/080

Why doesn’t Dispatcher implement ISynchronizeInvoke?

Posted by Rob

This is a rant.  I don't have the answer to the question.

So, the latest project I'm working on is a kiosk app in WPF that has to interact with hardware.  The hardware has various needs; some of it I need to poll, and others I check on status every given interval.  Every class I've created for interacting with the hardware has a SynchronizingObject property, just like the System.Timers.Timer class, and I was pretty happy with myself when I figured out that my event raising implementation was the same as that class's.

The SynchronizingObject property looks like this:

   1:  public ISynchronizeInvoke SynchronizingObject
   2:  {
   3:      get { return m_syncObj; }
   4:      set { m_syncObj = value; } 
   5:  }

Pretty straightforward.  To call an event it might be something like this:

   1:  protected virtual void OnStatusChanged(EventArgs e)
   2:  {
   3:      if (StatusChanged != null)
   4:      {
   5:          if (m_syncObj == null || !m_syncObj.InvokeRequired)
   6:              StatusChanged(this, e);
   7:          else
   8:              m_syncObj.BeginInvoke(StatusChanged, new object[] { this, e });
   9:      }
  10:  }

Easy?  Good.

Well, like apparently everything straightforward about Windows Forms programming, it's been changed for Windows Presentation Foundation.  Each Visual element has an associated Dispatcher; Dispatchers are created per-thread, and like in Windows Forms, you can't update a Visual or its descendent tree from another thread.  And the Dispatcher class almost looks like it would be interface-compatible with ISynchronizeInvoke with a couple exceptions.  I thought, "great!  I'll be able to just create a simple Adapter class!"  Nope.  Well, it wasn't simple, that's for sure.

Let's take a look at the overload list for Invoke and you might see why.  Invoked delegates either take one argument or one argument plus a params list of arguments.  Then you think.... what?

There are some issues with params lists.  For example, let's say that you're passing an argument that is an object[].  Should that get expanded?  Consider this code:

   1:  void DoStuff(params object[] args) { ... }
   2:   
   3:  // now within a function
   4:  object[] values = new object[] { 1, "stuff", 25.0 };
   5:  DoStuff(values, "something else?");

The kind of difficulty we run into is -- should "values" be expanded out or should it stay as an array?  In other words, should args be { 1, "stuff", 25.0, "something else?" }, or should it be { { 1, "stuff", 25.0 }, "something else?" }?  What about when it's the only argument passed - what if DoStuff(values) is the call?  Then should args be {1, "stuff", 25.0} or { {1, "stuff", 25.0} }?  For the purposes of this application, I assumed that, in the first case, args would be like the latter; and in the second case, args would be like the former.

So here's my test app -- it's a basic WPF application.  Here's Window1.xaml:

   1:  <Window x:Class="DispatcherTest.Window1"
   2:      xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
   3:      xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
   4:      Title="Window1" Height="300" Width="300">
   5:      <Grid>
   6:          <Button Height="23" Margin="102,87,100,0" Name="button1" VerticalAlignment="Top" Click="button1_Click">Button</Button>
   7:      </Grid>
   8:  </Window>

and Window1.xaml.cs:

   1:  public partial class Window1 : Window
   2:  {
   3:      private ISynchronizeInvoke m_invoker;
   4:   
   5:      public Window1()
   6:      {
   7:          InitializeComponent();
   8:          m_invoker = new DispatcherWinFormsCompatAdapter(this.Dispatcher));
   9:      }
  10:   
  11:      private void button1_Click(object sender, RoutedEventArgs e)
  12:      {
  13:          ThreadPool.QueueUserWorkItem(new WaitCallback(DoSomeLongRunningWork), null);
  14:      }
  15:   
  16:      private void DoSomeLongRunningWork(object state)
  17:      {
  18:          Thread.Sleep(2000);
  19:          EventHandler updateButton = delegate(object sender, EventArgs e)
  20:          {
  21:              button1.Content = "Clicked!";
  22:          };
  23:   
  24:          if (m_invoker.InvokeRequired)
  25:          {
  26:              m_invoker.BeginInvoke(updateButton, new object[] { this, EventArgs.Empty });
  27:          }
  28:          else
  29:          {
  30:              updateButton(this, EventArgs.Empty);
  31:          }
  32:      }
  33:  }

Finally, here's my skeleton adapter class.  Note that EndInvoke and Invoke are not used so I don't implement them yet:

   1:  internal class DispatcherWinFormsCompatAdapter : ISynchronizeInvoke
   2:  {
   3:      #region IAsyncResult implementation
   4:      private class DispatcherAsyncResultAdapter : IAsyncResult
   5:      {
   6:          private DispatcherOperation m_op;
   7:          private object m_state;
   8:   
   9:          public DispatcherAsyncResultAdapter(DispatcherOperation operation)
  10:          {
  11:              m_op = operation;
  12:          }
  13:   
  14:          public DispatcherAsyncResultAdapter(DispatcherOperation operation, object state)
  15:              : this(operation)
  16:          {
  17:              m_state = state;
  18:          }
  19:   
  20:          public DispatcherOperation Operation
  21:          {
  22:              get { return m_op; }
  23:          }
  24:   
  25:          #region IAsyncResult Members
  26:   
  27:          public object AsyncState
  28:          {
  29:              get { return m_state; }
  30:          }
  31:   
  32:          public WaitHandle AsyncWaitHandle
  33:          {
  34:              get { return null; }
  35:          }
  36:   
  37:          public bool CompletedSynchronously
  38:          {
  39:              get { return false; }
  40:          }
  41:   
  42:          public bool IsCompleted
  43:          {
  44:              get { return m_op.Status == DispatcherOperationStatus.Completed; }
  45:          }
  46:   
  47:          #endregion
  48:      }
  49:      #endregion
  50:      private Dispatcher m_disp;
  51:      public DispatcherWinFormsCompatAdapter(Dispatcher dispatcher)
  52:      {
  53:          m_disp = dispatcher;
  54:      }
  55:      #region ISynchronizeInvoke Members
  56:   
  57:      public IAsyncResult BeginInvoke(Delegate method, object[] args)
  58:      {
  59:          if (args != null && args.Length > 1)
  60:          {
  61:              object[] argsSansFirst = GetArgsAfterFirst(args);
  62:              DispatcherOperation op = m_disp.BeginInvoke(DispatcherPriority.Normal, method, args[0], argsSansFirst);
  63:              return new DispatcherAsyncResultAdapter(op);
  64:          }
  65:          else
  66:          {
  67:              if (args != null)
  68:              {
  69:                  return new DispatcherAsyncResultAdapter(m_disp.BeginInvoke(DispatcherPriority.Normal, method, args[0]));
  70:              }
  71:              else
  72:              {
  73:                  return new DispatcherAsyncResultAdapter(m_disp.BeginInvoke(DispatcherPriority.Normal, method));
  74:              }
  75:          }
  76:      }
  77:   
  78:      private static object[] GetArgsAfterFirst(object[] args)
  79:      {
  80:          object[] result = new object[args.Length - 1];
  81:          Array.Copy(args, 1, result, 0, args.Length - 1);
  82:          return result;
  83:      }
  84:   
  85:      public object EndInvoke(IAsyncResult result)
  86:      {
  87:          DispatcherAsyncResultAdapter res = result as DispatcherAsyncResultAdapter;
  88:          if (res == null)
  89:              throw new InvalidCastException();
  90:   
  91:          while (res.Operation.Status != DispatcherOperationStatus.Completed || res.Operation.Status == DispatcherOperationStatus.Aborted)
  92:          {
  93:              Thread.Sleep(50);
  94:          }
  95:   
  96:          return res.Operation.Result;
  97:      }
  98:   
  99:      public object Invoke(Delegate method, object[] args)
 100:      {
 101:          if (args != null && args.Length > 1)
 102:          {
 103:              object[] argsSansFirst = GetArgsAfterFirst(args);
 104:              return m_disp.Invoke(DispatcherPriority.Normal, method, args[0], argsSansFirst);
 105:          }
 106:          else
 107:          {
 108:              if (args != null)
 109:              {
 110:                  return m_disp.Invoke(DispatcherPriority.Normal, method, args[0]);
 111:              }
 112:              else
 113:              {
 114:                  return m_disp.Invoke(DispatcherPriority.Normal, method);
 115:              }
 116:          }
 117:      }
 118:   
 119:      public bool InvokeRequired
 120:      {
 121:          get { return m_disp.Thread != Thread.CurrentThread; }
 122:      }
 123:   
 124:      #endregion
 125:  }

All told, I'm still not sure that this will work with delegates with more than two parameters.  I'm still concerned about params argument binding -- but what can I do?  Reflection.Emit custom function calls for n parameters?  No thanks.

Microsoft guy who made System.Windows.Threading - for .NET 4.0, how about making Dispatcher implement ISynchronizeInvoke, hm?  Thanks!

Tagged as: , No Comments
27Mar/080

Model-View-Controller and Assembly Dependencies

Posted by Rob

The title of this blog post might be a slight misnomer because it isn't dealing with MVC directly.  It's dealing with something a little more high level (oddly enough).

I'm working on a personal project when I have time.  I haven't played around with Battle.net in a while and there's still a fairly active user community around it (not to mention that Starcraft 2 is going to be released one day).  On top of everything else, I learned so much about all kinds of topics (design patterns, reflection, UI design) when I was working on clients in college that I still think it's valuable.  My latest project is giving me the opportunity to leverage some new additions to .NET 3.5 -- the System.AddIns namespace -- which will be a first for me, and it's the first time I'm using Model-View-Controller to design the client.  The primary motivator in using MVC is that I want to also build up an AJAX-powered rich web interface, and so the use of MVC allows me to incorporate multiple controller listeners to the model's events, to which I can hook up a web service.  Finally, I want to just make clean software.  MVC seems to be a particularly good pattern for this project, so I'm going to run with it.

In addition to the fact that it makes sense to separate out the model from the view, I've run into a little snag.  It's taken me three days to work out the problem in my head, but I think I'm finally at a point where I can be satisfied with the solution.

One of the great things about .NET is its rich extensibility model exposed via attributes.  By allowing developers to create their own metadata to be used in assemblies, we can create contracts in such a way that the extension code can tell us about itself.  Now, of course, we can create contracts in C++, too, but arguably it's not as clean.  Where a plugin system for a C++ library might require the code to provide us with a naked function, one for C# or another .NET language might be exposed via an assembly-level attribute:

// C++
typedef IPluginFactory PLUGIN_FACTORY, *PPLUGIN_FACTORY;
__declspec(dllexport) extern "C" PPLUGIN_FACTORY GetPluginFactory();

// C#
[assembly: Plugin(typeof(MyPluginFactory))]

These are, of course, very similar, and they're used in a similar way.  The .NET app will know to look for that PluginAttribute on the assembly, and the C++ loader will know to look for an exported function called GetPluginFactory.  But I digress.

Among other things, I'd like in my app that the model classes be decorated with View-defined attributes.  For example, I plan on incorporating some of the features from Shiny Design into my app, and one of the things I'd like to do is create an adapter class to create a proxy for events.  Essentially, it's annoying to me to have to generate code that handles the configuration of events; specifically, each event type can have a color associated with it.  I'd decorate my event named "UserJoined" with [Name("User Joined Channel")] and [DefaultValue("Yellow", typeof(ColorConverter))].  It should know enough to enumerate the events, pull out the special data, and save to XML.  Then at runtime, access them via a special dictionary.

My dilemma was what to do about the dependencies.  Ultimately all of this will be open-source; I don't want there to be dependencies between my view classes and my model classes.  But how could I release my model classes without any dependency on my view classes if I wanted to have these attributes?

Then, tonight I realized: conditional compilation constants.

   1:  #if INCLUDE_VIEW_ATTRIBUTES
   2:  [Name("User Joined Channel")]
   3:  [DefaultValue("Yellow", typeof(ColorConverter))]
   4:  #endif
   5:  public event UserEventHandler UserJoined;

Done!

18Mar/080

Thoughts on Generics and Best Practices – Building Cohesive Software

Posted by Rob

In general, I don't know whether I internalized this as a best practice because it makes sense to me, or because I read it somewhere.  But I have this notion that there are only certain places in which it is appropriate to expose a generic member to the outside world, such as when the item is clearly generic in scope.  For example, if I wanted to implement a FIFO list to get the behavior of a queue but the performance characteristics of a list, I might call my class ListQueue<T>.  As a library class, it makes sense to have this as a generic type.  But what about when it would save programmer effort only?  I don't think that this is a valid reason to expose the class to the outside world.

Between my last two projects, I have about a dozen classes that look like this:

   1:      [ConfigurationCollection(typeof(SerialPortConfigurationElement), AddItemName = "SerialPort", CollectionType = ConfigurationElementCollectionType.BasicMap)]
   2:      public class SerialPortConfigurationElementCollection : ConfigurationElementCollection, IEnumerable<SerialPortConfigurationElement>
   3:      {
   4:          protected override ConfigurationElement CreateNewElement()
   5:          {
   6:              return new SerialPortConfigurationElement();
   7:          }
   8:   
   9:          protected override object GetElementKey(ConfigurationElement element)
  10:          {
  11:              return ((SerialPortConfigurationElement)element).PortName; 
  12:          }
  13:   
  14:          public override ConfigurationElementCollectionType CollectionType
  15:          {
  16:              get
  17:              {
  18:                  return ConfigurationElementCollectionType.BasicMap;
  19:              }
  20:          }
  21:   
  22:          public SerialPortConfigurationElement this[int index]
  23:          {
  24:              get { return BaseGet(index) as SerialPortConfigurationElement; }
  25:          }
  26:   
  27:          public SerialPortConfigurationElement this[string index]
  28:          {
  29:              get { return BaseGet(index) as SerialPortConfigurationElement; }
  30:          }
  31:   
  32:          #region IEnumerable<SerialPortConfigurationElement> Members
  33:   
  34:          public new IEnumerator<SerialPortConfigurationElement> GetEnumerator()
  35:          {
  36:              return new LightWrapperEnumerator<SerialPortConfigurationElement>(base.GetEnumerator());
  37:          }
  38:   
  39:          #endregion
  40:      }

This class enables the following type of configuration settings:

   1:      <SerialPorts>
   2:          <SerialPort PortName="COM6" BaudRate="9600" DataBits="8" Parity="None" StopBits="One" />
   3:          <SerialPort PortName="COM8" /> <!-- Only the PortName is required.  The others have defaults. -->
   4:      </SerialPorts>

This is all well and good.  However, as I look at my class, I see that it would be fairly easy to rip out a lot of that functionality and make it a generic class.  LightWrapperEnumerator<T> is already something like that - it's an internal-only class that basically wraps an IEnumerator into an IEnumerator<T>.  That's fairly easy to hide from the outside world, though; I only expose it to calling code using IEnumerator<T> and nobody is the wiser unless they call GetType() on it, which is unlikely.  I realized as I built the second duplicate of this class today involving a Motor configuration element, that I could rip out the guts and implement everything EXCEPT GetElementKey and have it out there:

   1:  abstract class ChildlessConfigurationElementCollection<T> : ConfigurationElementCollection 
   2:      where T : ConfigurationElement, new()
   3:  {
   4:      protected override ConfigurationElement CreateNewElement()
   5:      {
   6:          return new T();
   7:      }
   8:   
   9:      public override ConfigurationElementCollectionType CollectionType
  10:      {
  11:          get
  12:          {
  13:              return ConfigurationElementCollectionType.BasicMap;
  14:          }
  15:      }
  16:   
  17:      public T this[int index]
  18:      {
  19:          get { return BaseGet(index) as T; }
  20:      }
  21:   
  22:      public T this[string index]
  23:      {
  24:          get { return BaseGet(index) as T; }
  25:      }
  26:   
  27:      #region IEnumerable<SerialPortConfigurationElement> Members
  28:   
  29:      public new IEnumerator<T> GetEnumerator()
  30:      {
  31:          return new LightWrapperEnumerator<T>(base.GetEnumerator());
  32:      }
  33:   
  34:      #endregion
  35:  }

Implementing the serial port collection then, would be VERY easy:

   1:  [ConfigurationCollection(typeof(SerialPortConfigurationElement), AddItemName = "SerialPort", CollectionType = ConfigurationElementCollectionType.BasicMap)]
   2:  class SerialPortConfigurationElementCollection : ChildlessConfigurationElementCollection<SerialPortConfigurationElement>
   3:  {
   4:      protected override object GetElementKey(ConfigurationElement element)
   5:      {
   6:          return ((SerialPortConfigurationElement)element).PortName;
   7:      }
   8:  }

However, aside from the fact that they have the same "programmer functionality," there's not much motivating me to do anything else.

Hopefully, as you're looking at these two examples, you're saying "that doesn't look correct."  From an object model perspective, it doesn't make much sense, even in the abstract world (away from objects), to have even a concept of a "Childless Configuration Element Collection."  What does it mean, anyway?  Would it make sense for me to cast a configuration property object to a ChildlessConfigurationElementCollection of T?  I... don't think so.

I've been reading Code Complete 2nd Ed. lately.  I can't remember whether I've blogged about my respect for Steve McConnell before, but this guy is simply an amazing contributor to our craft.  In it, he talks at length about a software quality attribute that we'd never really discussed in school: cohesion.

The Wikipedia article gives it a great treatment, so I won't really delve into the depths of what cohesion means and how it can be measured (buy the book!).  As I look at this type of code, though, I can only see some kind of cross between procedural and logical cohesion - not exactly excellent in the scale.  The real bottom line is that these all operate on different data - how are we to treat them the same?

Incidentally, the way that C++ implements templates (not C++/CLI generics) would make this not be so terrible.  I could define this template class and it would actually generate the C++ code (Java actually would do this in an adequate way, as well).  Because the code is generated at compile-time, I don't need to expose the metadata about the template class, and so I'm saved in that regard.  Unfortunately, if I were to define this class in C# and need to access it in an external assembly, I'd need to mark the base class as public, and so everything else in the world can inherit from or create references to variables of type ChildlessConfigurationElementCollection<T>.  It doesn't make sense to have this type exposed; it only makes sense at the higher level. 

So I am left with the decision: do I duplicate code or expose bad, incomplete objects to the world?  I'll duplicate code, thank you very much.

3Mar/080

Exceptional Exception Handling: Taking Exception with the Community

Posted by Rob

I could have gone on with the title for a while longer.

Frequently when I'm working on an application, it occurs to me that the user doesn't care about the technical reasons behind an exceptional condition.  Typically, the user simply cares that something went wrong; or, sometimes, the user doesn't care, especially if you can make the program recover gracefully.

Since the only .NET-sanctioned way to handle errors is through structured exception handling, and since I love SEH, I don't want to be too hard on it.  But the truth is, there are certainly instances where "On Error Resume Next" would be nice.

Consider the following constructor:

   1:          public ConnectionBase(string server, int port)
   2:          {
   3:              m_server = server;
   4:              m_port = port;
   5:              try
   6:              {
   7:                  m_ipep = new IPEndPoint(Dns.GetHostEntry(server).AddressList[0], port);
   8:              }
   9:              catch (SocketException) { }
  10:          }

This class essentially encapsulates a TcpClient.  Now, what I do here in the constructor is attempt to resolve the host address.  In the event that there is an error, for whatever reason, Dns.GetHostEntry will raise a SocketException, typically telling me that the URI could not be found.

Now, the general wisdom about SEH is to not just "eat" an exception; rather, we prefer to do something worthwhile with it.  But in this case, I haven't invalidated the state of my class, I haven't invalidated the state of the system, or done anything to harm my class.  In fact, if the m_ipep (an IPEndPoint object) isn't initialized when Connect() is called, it attempts to do it as well:

   1:          public virtual bool Connect()
   2:          {
   3:              // ...
   4:              if (m_ipep == null)
   5:              {
   6:                  try
   7:                  {
   8:                      m_ipep = new IPEndPoint(Dns.GetHostEntry(m_server).AddressList[0], m_port);
   9:                  }
  10:                  catch (SocketException se)
  11:                  {
  12:                      OnError(string.Format("Your computer was unable to resolve hostname {0}.  If necessary, add an entry to %SystemRoot%\\system32\\drivers\\etc\\hosts, or flush your DNS resolver cache, and try again.",
  13:                          m_server), se);
  14:                      return false;
  15:                  }
  16:              }
  17:              //...
  18:              return true;
  19:          }

This method correctly bubbles an error to the using classes in the event that the IP end point could not be located.  You might ask, though - what is the advantage of using early resolution?  That is, I could drop the exception code altogether.

Well, if someone is debugging their code using my library and has first-chance exception handling enabled, it is easy enough to spot where a parameter might be causing issues.

Once the class is initialized, someone using a debugger can see the internals of my class, including the resolved end point.  This may help them identify the remote end point's IP address; perhaps that particular server is not functional.

There are other issues with the "best practices" as well -- for instance, they are sometimes at odds with each other.  The .NET Framework Guidelines say not to overuse catch, but also to not to just catch a general exception.  Consider:

   1:              try
   2:              {
   3:                  // create an instance of this variable to make sure that the data provider can be loaded.
   4:                  DataProvider provider = DataProvider.Instance;
   5:              }
   6:              catch (TypeLoadException tle)
   7:              {
   8:                  throw new ConfigurationErrorsException(
   9:                      string.Format("Type '{0}' was not a valid type according to the runtime.", keyProviderType), tle);
  10:              }
  11:              catch (TargetInvocationException tie)
  12:              {
  13:                  throw new ConfigurationErrorsException(
  14:                      string.Format("An exception was raised in the constructor of type '{0}'.  More information may be available in the contained exception.", keyProviderType), tie.InnerException);
  15:              }
  16:              catch (MethodAccessException mae)
  17:              {
  18:                  throw new ConfigurationErrorsException(
  19:                      string.Format("Security policy prevented access to the default public constructor of type '{0}'.", keyProviderType), mae);
  20:              }
  21:              catch (MissingMethodException mme)
  22:              {
  23:                  throw new ConfigurationErrorsException(
  24:                      string.Format("No default public constructor existed on the type '{0}'.  Change the application configuration so that the 'dataProvider' property of the 'dataConfiguration' section points to a type with a default public constructor and inherits from the '{1}' type.", keyProviderType, typeof(KeyProvider)), mme);
  25:              }
  26:              catch (MemberAccessException memae)
  27:              {
  28:                  throw new ConfigurationErrorsException(
  29:                      string.Format("Could not create an instance of the abstract type '{0}'.  Change the application configuration so that the 'dataProvider' property of the 'dataConfiguration' section does not indicate an abstract type.", keyProviderType), memae);
  30:              }

In the code above, I have legitimate reasons for catching these - retrieving the DataProvider.Instance property is documented to raise these exceptions, which are in turn raised by Activator.CreateInstance.  The real problem with this kind of situation is that the outer calling code needs to be shielded, and so ultimately, the calling code handles only a ConfigurationErrorsException.  This in turn is handled and logged to the event log before terminating the Windows Service that hosts this code.

You can read more in the .NET Developer's Guide - Exception Handling article on MSDN.  Just bear in mind - you probably want different exception handling strategies if you're writing a user interface vs. library code vs. data access code.  At the end of the day, it's how much bad stuff you want to show your users, how much bad stuff you can handle internally to your application, and how much bad stuff you're willing to let your application produce because you over-caught exceptions.