CODEWITHSUNDEEP
c#multithreadingconcurrency
sebas
sebas

Reputation: 1085

Using ConcurrentBag correctly

Edit: Thank you, you made me realise that the code below is not working as I assumed, since somehow I thought that cbag works like a hashset. Sorry about it, you saved me some headache :)

the following function is the only function that can change _currentSetOfStepsProcessing. This function can be called from different threads. I am not sure if I understood correctly the use of a ConcurrentBag, so please let me know if in your opinion this can work. _stepsToDo datastructure is never modified once the process starts.

void OnStepDone(InitialiseNewUserBase obj)
    {
        var stepToDo = _stepsToDo[_currentSetOfStepsProcessing];

        stepToDo.TryTake(out obj);

        if (stepToDo.Count == 0) //can I assume it will enter here once per ConcurrentBag?
        {
            if (_currentSetOfStepsProcessing < _stepsToDo.Count - 1)
            {
                _currentSetOfStepsProcessing++;
            }
        }
    }

    List<ConcurrentBag<InitialiseNewUserBase>>      _stepsToDo = new List<ConcurrentBag<InitialiseNewUserBase>>();
    Action                                          _onFinish;
    int                                             _currentSetOfStepsProcessing;

Upvotes: 1

Views: 827

Answers (1)

usr
usr

Reputation: 171178

  1. stepToDo.TryTake(out obj); might fail, you don't handle that.
  2. Why are you out-referencing the method argument? This simply overwrites the argument. Why take an argument if you throw it away? More likely, this is a misunderstanding of some kind.
  3. can I assume it will enter here once per ConcurrentBag since access to the bag is apparently concurrent multiple accessing threads might see 0. So yes, you need to handle that case better.

Probably, you should not make things so difficult and use lock in combination with non-concurrent data structures. This would only be a good idea if there was a high frequency of bag operations which seems unlikely.

What about this:

foreach (/*processing step*/) {
 Parallel.ForEach(/*item in the step*/, x => { ... });
}

Much simpler.

Upvotes: 1

Related Questions