Using locks doesn't prevent Collection was modified; enumeration operation may not execute

Question

This question has been asked many times, but this is a special case.

public class JobStatusMonitor
{
    private static List<Job> _runningJobs = new List<Job>();
    private static object myLock = new object();

    public static void AddJob(GPSJob input)
    {
        lock (myLock)
            _runningJobs.Add(input);
    }

    public static void Start(int pollInterval)
    {
        while (true)
        {
            var removeJobs = new List<GPSJob>();
            lock (myLock)
            {
                foreach (var job in _runningJobs)
                {
                    if (job.IsComplete())
                    {
                        removeJobs.Add(job);
                    }
                }
            }

            foreach (var job in removeJobs)
            {
                _runningJobs.Remove(job);
            }

            System.Threading.Thread.Sleep(pollInterval);
        }
    }
}

The list _runningJobs is private, so nothing outside of this class can modify it unless it uses the AddJob method. The AddJob method uses the same lock as the foreach loop so it shouldn't be able to modify the collection while it is iterating.

My understanding of what should be happening is Start(5000) gets called, there is nothing in the list so it skips to Thread.Sleep(). Background processes add jobs to the list. The while loop returns to the foreach loop and applies the lock. While iterating over the list any other threads that attempt to add to the collection wait until iteration is complete. As soon as iteration completes each of those threads will add their job, the lock will not cause race conditions even if there are many threads attempting to add jobs.

What actually happens is any job that is added while this thread is sleeping is added successfully. Jobs that are added while this list is iterating do not wait for iteration to complete despite the lock.

Why doesn't the lock prevent this error?

EDIT: Copy to a new list within the lock eliminates the error.

public class JobStatusMonitor
{
    private static List<Job> _runningJobs = new List<Job>();
    private static object myLock = new object();

    public static void AddJob(GPSJob input)
    {
        lock (myLock)
        {
           _runningJobs.Add(input);
        }
    }

    public static void Start(int pollInterval)
    {
        while (true)
        {

            lock (myLock)
            {
                var completeJobs = _runningJobs.Where(job => job.IsComplete()).ToList();
                foreach (var job in completeJobs)
                {
                    _runningJobs.Remove(job);
                    job.TaskCompletionSource.SetResult(null);
                }       
            }

            System.Threading.Thread.Sleep(pollInterval);
        }
    }
}

Answer 1

The problem was spotted by @Philipe, you're modifiying the list outside of the lock. You should have all your modifying calls protected by the lock.

To simplify, you can just compute the new list of non completed job and swap with the current job list inside the lock. Something along the lines of:

lock(myLock) {
    var newRunningJobs = _runningJob.Where(j => !Job.IsComplete(j)).ToList();
    _runningJob = newRunningJobs;
}

Answer 2

Your call to _runningJobs.Remove(job); is not in a lock. The collection can't be aware of it, and you are removing items from it while potentially having a lock on it in AddJob. Move the removal of jobs in a lock or wrap it in one and it should solve your problem.