1
\$\begingroup\$

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);
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ it is unclear what you want us to review, if you would like an overall review please state this in the question please. Code-Only Questions are frowned upon, on Stack Exchange sites. \$\endgroup\$ – Malachi Oct 25 '13 at 20:43
  • \$\begingroup\$ sorry, I had a feeling this site was more informal than so but I'm sure that even here there are standards. thx \$\endgroup\$ – Chris Hayes Oct 25 '13 at 21:14
  • \$\begingroup\$ thank you, remember that the longer the code snippet you give us to review the longer it might take for reviews. it is okay to post several questions with shorter code snippets, you can even add links to the other questions, we don't mind that. \$\endgroup\$ – Malachi Oct 25 '13 at 21:17
  • \$\begingroup\$ So many namespaces … Maybe you should split this into multiple files? Or is that just for posting here? \$\endgroup\$ – svick Oct 25 '13 at 22:00
1
\$\begingroup\$
  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.
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ What is subtly broken about the singleton implementation? It's almost a verbatim copy of the Multithreaded Singleton implementation in the article you posted. Note that lock is an implicit full memory barrier. On a related note, it might be useful to compare singleton implementations with this answer. \$\endgroup\$ – ta.speot.is Oct 26 '13 at 23:40
  • \$\begingroup\$ @ta.speot.is: Yes, "almost". If you read the linked article again you will find this "Also, the variable is declared to be volatile to ensure that assignment to the instance variable completes before the instance variable can be accessed" \$\endgroup\$ – ChrisWue Oct 27 '13 at 7:42
  • \$\begingroup\$ You are correct. \$\endgroup\$ – ta.speot.is Oct 27 '13 at 7:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.