пятница, 11 января 2013 г.

A little unobvious error


Here I describe one error I've done which might seem not obvious at first sight. (The code in this post is C#)

The following situation occured at my work recently. Some class was declared approximately as follows:

    public class SomeClass
    {
        private volatile int _x1; 
        private volatile int _x2;


        public int X1
        {
            get { return _x1; }
            set
            {
                System.Threading.Interlocked.Exchange(ref _x1, value);
                CheckForDone();
            }
        }
        public int X2
        {
            get { return _x2; }
            set
            {
                System.Threading.Interlocked.Exchange(ref _x2, value);
                CheckForDone();
            }
        }
        public int X { get; set; }


        public bool IsCompleted { get { return X1 == X && X2 == X; } }


        private void CheckForDone()
        {
            if (IsCompleted)
                RaiseDone(new EventArgs());
        }


        public event EventHandler Done;
        private void RaiseDone(EventArgs args)
        {
            if (Done != null)
                Done(this, args);
        }
    }

Here is the situation: variables _x1 and _x2 are incremented in some unknown order (from different threads) until both of them reach value X. When each variable achieves new value, CheckForDone function checks whether both values have reached X and if they had then some event is fired. Thus last variable which reaches X fires this event.

The problem is that Done event fires twice sometimes (this is undesirable behavior). This often happened with first actuation of Done event - due to jitter lags, I think. The reason is, of course, that IsCompleted and setters of X1 and X2 are not some atomic operations. Then first thread calculates X1 == X (which is, say, true), X2 changes (X2 == X becomes true) and fires another CheckForDone function. In this case IsCompleted returns true in both cases and event fires twice.

There are several ways to solve the problem and I've chosen the simplest one: I added a variable which indicates whether Done event had already fired or not.

        private volatile int _doneFired = 0;
        public bool DoneFired
        {
            get { return _doneFired == 1; }
            set { System.Threading.Interlocked.Exchange(ref _doneFired, value ? 1 : 0); }
        }

Then CheckForDone is modified in following way:

        private void CheckForDone()
        {
            if (IsCompleted && !DoneFired)
            {
                // problem zone
                DoneFired = true;
                RaiseDone(new EventArgs());
            }
        }

It was obvious enough but here are some errors and, surprisingly, the problem occured again.

The error here is that both threads can occur in the "problem zone" before one of them had set DoneFired to true. The solution is to use CompareExchange instead of Exchange method that guarantees that both comparing to true and setting to true is performed as atomic operation. So the code for DoneFired has to be written in the form:

        public bool DoneFired 
        { 
            get { return System.Threading.Interlocked.CompareExchange(ref _doneFired, 1, 0) == 1; } 
        }

        private void CheckForDone()
        {
            if (IsCompleted && !DoneFired)
            {
                RaiseDone(new EventArgs());
            }
        }

Sure, the problem can be solved in another, more obvious way - by introducing a lock on some object that guarantees that DoneFired will check for true and change only once, but lock works significantly slower and this can be crucial in some scenarios.

Комментариев нет:

Отправить комментарий