Simple WCF messaging system via EF

Question

I'm open to any comments on this code/approach. This is mostly architecture and threading.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

using System.ComponentModel;
using System.ServiceModel;
using System.ServiceProcess;
using System.Configuration;
using System.Configuration.Install;
using System.Runtime.Serialization;
using System.ComponentModel.DataAnnotations;
using System.Data.Entity;
using System.Security.Permissions;

using System.Diagnostics;
using System.Timers;


namespace Mobwt.MessageService
{
    [ServiceContract]
    public interface IStepMessageCallback
    {


        [OperationContract]
        void Callback(StepMessage message);

        [OperationContract]
        List<StepMessage> GetMessages(string JobId, bool unreadonly);


        // TODO: Add your service operations here
    }

    public class StepMessageCallback : IStepMessageCallback
    {
        [PrincipalPermission(SecurityAction.Demand, Role = "mobwta_admin")]
        public void Callback(StepMessage message)
        {
            if (message != null)
            {
                DataManager.AddMessage(message);
                //using (MessageContext db = new MessageContext())
                //{
                //    message.FlashRead = false;
                //    db.Messages.Add(message);
                //    db.SaveChanges();
                //}
            }
        }

        [PrincipalPermission(SecurityAction.Demand, Role = "mobwta_admin")]
        public List<StepMessage> GetMessages(string JobId, bool unreadonly = true)
        {
            return DataManager.ReadMessages(JobId, unreadonly);

            //List<StepMessage> msgs = null;
            //using (MessageContext db = new MessageContext())
            //{
            //    var msgqry = db.Messages.Where(m => unreadonly ? !m.FlashRead && m.JobId == JobId : m.JobId == JobId).Take(10);
            //    msgs = msgqry.ToList();
            //    foreach (var m in msgqry)
            //        m.FlashRead = true;
            //    db.SaveChanges();
            //}
            //return msgs;
        }
    }

    public static class DataManager
    {
        static object lockobj = new object();

        static int deadcount = 0;

        static int deadreadcount = 0;

        static Timer _saveTimer;

        static Timer _readTimer;

        static List<StepMessage> messages;
        static List<StepMessage> Messages
        {
            get
            {
                if (messages == null)
                {
                    lock (lockobj)
                    {
                        if (messages == null)
                            messages = new List<StepMessage>();
                    }
                }
                return messages;
            }
        }

        static MessageContext saveContext;
        static MessageContext SaveContext
        {
            get
            {
                if (saveContext == null)
                {
                    saveContext = new MessageContext();
                }
                return saveContext;
            }
        }

        static MessageContext readContext;
        static MessageContext ReadContext
        {
            get
            {
                if (readContext == null)
                {
                    lock (lockobj)
                    {
                        if (readContext == null)
                        {
                            readContext = new MessageContext();
                            deadreadcount = 0;
                            if (_readTimer == null)
                                InitializeReadTimer();
                        }
                    }
                }
                return readContext;
            }
        }

        static void _timer_Elapsed(object sender, ElapsedEventArgs e)
        {
            if (Messages.Count > 0)
            {
                try
                {
                    List<StepMessage> cMsgs;
                    lock (lockobj)
                    {
                        cMsgs = Messages.ToList();
                        Messages.Clear();
                    }
                    SaveContext.Messages.AddRange(cMsgs);
                    SaveContext.SaveChanges();
                    deadcount = 0;
                }
                catch (Exception ex)
                {
                }
            }
            else
            {
                deadcount++;
                if (deadcount > 20)
                {
                    DisposeTimer(_saveTimer);
                    DisposeContext();
                    deadcount = 0;
                }
            }
        }

        private static void DisposeContext()
        {
            SaveContext.Dispose();
            saveContext = null;
        }

        private static void DisposeTimer(Timer timer)
        {
            if (timer != null)
            {
                if (timer.Enabled)
                    timer.Stop();
                timer.Dispose();
                timer = null;
            }
        }

        public static bool AddMessage(StepMessage message)
        {
            try
            {
                if (_saveTimer == null)
                    InitializeSaveTimer();
                lock (lockobj)
                {
                    Messages.Add(message);
                }
                return true;
            }
            catch
            {
                return false;
            }
        }

        public static List<StepMessage> ReadMessages(string jobid, bool unreadonly)
        {
            deadreadcount = 0;
            List<StepMessage> msgs = null;
            try
            {
                lock (lockobj)
                {
                    var msgqry = ReadContext.Messages.Where(m => unreadonly ? !m.FlashRead && m.JobId == jobid : m.JobId == jobid).Take(10);
                    if (msgqry.Count() > 0)
                    {
                        msgs = msgqry.ToList();
                        foreach (var m in msgqry)
                            m.FlashRead = true;
                        ReadContext.SaveChanges();
                    }
                }
            }
            catch (Exception ex)
            {
            }
            return msgs;
        }

        private static void InitializeReadTimer()
        {
            _readTimer = new Timer(500);
            _readTimer.Elapsed += _readTimer_Elapsed;
            _readTimer.Start();
        }

        static void _readTimer_Elapsed(object sender, ElapsedEventArgs e)
        {
            deadreadcount++;
            if (deadreadcount > 20 && readContext != null)
            {
                DisposeTimer(_readTimer);
                readContext.Dispose();
                readContext = null;
            }
        }

        private static void InitializeSaveTimer()
        {
            _saveTimer = new Timer(500);
            _saveTimer.Elapsed += _timer_Elapsed;
            _saveTimer.Start();
        }
    }

    [DataContract]
    public enum ProgressState
    {
        [EnumMember]
        Running,
        [EnumMember]
        Idle,
        [EnumMember]
        Success,
        [EnumMember]
        Failed,
        [EnumMember]
        Error
    }

    [DataContract]
    public class StepMessage
    {
        [DataMember]
        [Key]
        public int StepMessageID { get; set; }

        [DataMember]
        public string StepId { get; set; }

        [DataMember]
        public string JobId { get; set; }

        [DataMember]
        public string StepName { get; set; }

        [DataMember]
        public string Message { get; set; }

        [DataMember]
        public int Progress { get; set; }

        [DataMember]
        public ProgressState JobState { get; set; }

        [DataMember]
        public ProgressState StepState { get; set; }

        [DataMember]
        public DateTime StepStart { get; set; }

        [DataMember]
        public DateTime StepEnd { get; set; }

        [DataMember]
        public DateTime JobStart { get; set; }

        [DataMember]
        public DateTime JobEnd { get; set; }

        [DataMember]
        public DateTime TimeStamp { get; set; }

        [DataMember]
        public bool FlashRead { get; set; }

    }

    public class MessageContext : DbContext
    {
        public MessageContext() : base("MobileBilling") { }
        public DbSet<StepMessage> Messages { get; set; }
    }

    public class MessageService : ServiceBase
    {
        public ServiceHost host = null;

        public MessageService()
        {
            ServiceName = ConfigurationManager.AppSettings["ServiceName"];
        }

        public static void Main()
        {
            ServiceBase.Run(new MessageService());
        }

        protected override void OnStart(string[] args)
        {
            if (host != null)
                host.Close();

            host = new ServiceHost(typeof(StepMessageCallback));

            host.Open();
        }

        protected override void OnStop()
        {
            if (host != null)
            {
                host.Close();
                host = null;
            }
        }

    }


    [RunInstaller(true)]
    public class ProjectInstaller : Installer
    {
        private ServiceProcessInstaller proc;
        private ServiceInstaller svc;

        public ProjectInstaller()
        {
            proc = new ServiceProcessInstaller();
            proc.Account = ServiceAccount.LocalSystem;
            svc = new ServiceInstaller();
            svc.ServiceName = ConfigurationManager.AppSettings["ServiceName"];
            Installers.Add(proc);
            Installers.Add(svc);
        }
    }
}

Answer 1

1. Your double checked lock implementation for messages is subtly broken (the field should be volatile in order to create a memory barrier, this MSDN explains it with singleton as example but it is valid for any other field as well). It's also completely unnecessary:

   static List<StepMessage> messages = new List<StepMessage>();
   
   static List<StepMessage> Messages
   {
       get { return messages; }
   }
   

   Less code, less clever (clever is bad) and thread safe.

2. Some goes for your readContext except that you can't do the static init as it's destroyed occasionally. I'd stick to a simple lock unless it's proven to be a performance problem.
3. deadreadcount++ is not atomic. You do that a timer callback while assigning it in ReadMessage which can lead to the assignment of 0 being overwritten. Use Interlocked* methods to increment/decrement shared counters.
4. You swallow exceptions in many places. This is usually bad because the caller will have no idea that something went wrong when it does. I sincerely hope that you at least log them.
5. Returning null from a method (ReadMessages) which returns a collection is a pain in the butt as the caller as to specifically check for that. Returning an empty list is much nicer from a service consumer point of view.
6. It is not obvious from your code why lockobj protects both Messages and readContext.
7. In _readTimer_Elapsed you dispose of the read context while potentially accessing it in ReadMessage. The end result will be that the caller of ReadMessage will not get any result. It also means that ReadMessage was called in about 10sec intervals so I guess it doesn't matter that much for your use case but it's still ugly. It's bound to cause weird problems in the future when the code is extended.