1
\$\begingroup\$

My code fetches telegrams from a server periodically in the background. The server API is documented here. What do you think of the way I do multithreading by handling my _updatesList?

public class AutoUpdate
{
    public bool IsStarted { get; private set; }
    private readonly Timer _timer;

    private readonly List<Update> _updatesList;
    private readonly object _lock = new object();
    private long _lastUpdateId;

    public AutoUpdate()
    {
        _lastUpdateId = 0;
        _updatesList = new List<Update>();
        TimerCallback getUpdates = GetUpdates;
        _timer = new Timer(getUpdates, null, Timeout.Infinite,Timeout.Infinite);
        IsStarted = false;
    }

    public bool Start()
    {
        if (Monitor.TryEnter(_lock, 500))
        {
            try
            {
                _timer.Change(0, 100);
                IsStarted = true;
            }
            finally
            {
                Monitor.Exit(_lock);
            }
            return true;
        }
        return false;
    }

    public bool Stop()
    {
        if (Monitor.TryEnter(_lock, 500))
        {
            try
            {
                _timer.Change(Timeout.Infinite, Timeout.Infinite);
                IsStarted = false;
            }
            finally
            {
                Monitor.Exit(_lock);
            }
            return true;
        }
        return false;
    }

    public bool Terminate()
    {
        var stop = Stop();
        if (!stop)
            return false;
        _timer.Dispose();
        return true;
    }

    public ICollection<Update> GetLastUpdates()
    {
        lock (_lock)
        {
            //Make A Copy: for thread safety
            //Then Pass To ReadOnlyCollection
            return new ReadOnlyCollection<Update>(_updatesList.ToArray());
        }
    }

    public void RemoveFromUpdates(Update update)
    {
        lock (_lock)
        {
            _updatesList.Remove(update);
        }
    }

    private void GetUpdates(object state)
    {
        ReturnedResult<List<Update>> responseObject = null;

        if (Monitor.TryEnter(_lock))
        {
            try
            {
                HttpClient hc = new HttpClient();
                HttpContent requestContent = new ObjectContent<UpdateRequest>(new UpdateRequest(),
                    new JsonMediaTypeFormatter());
                var task =
                    hc.PostAsync(
                        "https://api.telegram.org/bot<Private Token>/getUpdates",
                        requestContent);
                if (task != null)
                {
                    var result = task.Result;
                    var jsonString = result.Content.ReadAsStringAsync().Result;
                    responseObject = JsonConvert.DeserializeObject<ReturnedResult<List<Update>>>(jsonString, new JsonSerializerSettings
                    {
                        ContractResolver = new CustomPropertyNamesContractResolver { Case = IdentifierCase.UnderscoreSeparator}
                    });
                }

                //Add To The List...
                if (responseObject != null && responseObject.Ok)
                {
                    var updateList = responseObject.Result.Where(w => w.UpdateId > _lastUpdateId);

                    //Resharper Detected: Possible multi enumeration of same list in same time
                    var list = updateList as Update[] ?? updateList.ToArray();

                    var maxId = list.Max(m => (long?) m.UpdateId) ?? 0;
                    _lastUpdateId = Math.Max(maxId, _lastUpdateId);

                    _updatesList.AddRange(list);
                }
            }
            finally
            {
                Monitor.Exit(_lock);
            }
        }
    }
}
\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

Disposable

You class manages a Timer, which you dispose of if Terminate is called. It feels like your class should implement IDisposable and cleanup the timer if it hasn't already been done.

Is Started

Does this need to be exposed as a public property? You're setting it in both your Start and Stop methods, but you never check its existing state. Is calling Start twice in a row without calling Stop acceptable? Perhaps Start should return IsStarted, rather than a value to indicate if it was able to acquire the lock.

Uncertain Usage

As I said in my response to your previous code it is unclear what your use case for this code is. You are protecting the list, by creating a copy of it in GetLastUpdates, however if this is called by multiple threads each thread could end up with its own copy of the underlying list with the same items in it. Each thread could then decide to perform some processing on the same item in the list. Does this matter? Is it prevented in someway outside of the code you've shown? Some alternative options would be:

  • Calling threads simply pop the first item from the list, then perform processing on it. If the underlying _updateList was a ConcurrentQueue and it was the the only way to fetch items, you could change GetUpdates to enqueue items and probably do away with the Monitor lock altogether. Using your existing data structure you would have something like this.

    public Update GetFirstItem()
    {
        Update update = null;
    
        if (Monitor.TryEnter(_lock))
        {
            try
            {
                update = _updatesList.FirstOrDefault();
                if (null != update)
                {
                    _updatesList.Remove(update);
                }
            }
            finally
            {
                Monitor.Exit(_lock);
            }
        }
        return update;
    }
    
  • Calling threads pass in a selection delegate to select the item from the list they need to remove, then perform processing on it.

    public Update GetFirstMatchingUpdate(Func<Update, bool> searchCriteria)
    {
        Update update = null;
    
        if (Monitor.TryEnter(_lock))
        {
            try
            {
                update = _updatesList.Where(searchCriteria).FirstOrDefault();
                if(null != update)
                {
                    _updatesList.Remove(update);
                }
            }
            finally
            {
                Monitor.Exit(_lock);
            }
        }
        return update;
    }
    

    Called like this:

    var update = au.GetFirstMatchingUpdate(x => x.SomeField == "someText" && x.Id > 3);
    
  • Calling threads call existing GetLastUpdates, find the update they want to process, remove it from the list, then perform processing only if that removal works (i.e. they were the first to remove it).

  • Processing is only actually performed by a single thread (with the contention being between the update and the processor), in which case the problem I've described doesn't exist.
\$\endgroup\$
4
  • \$\begingroup\$ Disposableit's been said inside timer document that the user should call dispose, i have lot of time take look at dispose implementation, but after several time in these years, i still am wonder what is that things... Is Started well, you are right. Uncertain Usage As inside my thread i don't do special fetching, you are right by all mean, and that's where i want to reconsider it.. A. Can Be right B. what can be a selection delegate? you mean it register event for when it need item? and how? C. It's my current way of processing an item D. Next Comment \$\endgroup\$ Jun 16, 2016 at 9:07
  • \$\begingroup\$ D. Well The timer can be single threaded, but not others \$\endgroup\$ Jun 16, 2016 at 9:07
  • \$\begingroup\$ @deadManN I've added a code example to illustrate what I mean by a selection delegate. \$\endgroup\$
    – forsvarir
    Jun 16, 2016 at 9:31
  • \$\begingroup\$ thank you. after 1 day holiday i think it's time time to mark yours as an asnwer \$\endgroup\$ Jun 18, 2016 at 4:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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