GeezerVison

This field is different from array "_syncObjects" only by last letter - it is confusing.
It would be good to either add comment for "_syncObjects" array about what it is; or rename "_syncObject"; or both.

Andrew (@anmenaga): Actually, after reviewing this logic again, _syncObject is insufficient since it only guards Clear(). Additionally, _syncObjects also appears to be fragile; it guards against individual usage of a given element but assumes the elements are valid; which is the actual problem.

My thinking is _syncObjects should be removed and _syncObject used to serialize access to the set instead of individual members.

Thoughts?

Steve Lee (@SteveL-MSFT), Dongbo Wang (@daxian-dbw), Travis Plunk (@TravisEz13) Guys, would you take a look at this class. My inclincation is to remove the individual locks and lock the set.

Dan Travison (@dantraMSFT) I'm all for simplification, but there should be a reason why they decided to go more complex 'individual-sync-object' route in the first place... Maybe something will deadlock with a single syncObject, and parallel "Add"s won't be able to execute for different DataPriorityTypes (not sure how critical this really is).

Actually, after reviewing this logic again, _syncObject is insufficient since it only guards Clear().

_dataSyncObjects and _dataToBeSent are used in multiple methods, but it appears that there is an assumption that those methods won't be called until the Fragmentor property is set (where both _dataSyncObjects and _dataToBeSent would be initialized). With this assumption, I think it's fine to directly use _dataSyncObjects and _dataToBeSent in methods other than Clear(). But we probably should add a comment about that assumption.

For Clear(), it's possible to be called before Fragmentor being set (in error path), so it's necessary to check _dataSyncObjects for null beforehand.

This assert should be removed because apparently _dataToBeSent can be null in that error path.

Never mind this comment. It's certainly right to assume _dataToBeSent != null when null != _dataSyncObjects 😄

Will do.

fixed

I think we can avoid the sync object _syncObject by the following code:

if (null != _dataSyncObjects)
{
    int promptResponseIndex = (int)DataPriorityType.PromptResponse;
    int defaultIndex = (int)DataPriorityType.Default;

    if (_dataToBeSent[promptResponseIndex] != null)
    {
        lock (_dataSyncObjects[promptResponseIndex])
        {
            if (_dataToBeSent[promptResponseIndex] != null)
            {
                _dataToBeSent[promptResponseIndex].Dispose();
                _dataToBeSent[promptResponseIndex] = null;
            }
        }
    }

    if (_dataToBeSent[defaultIndex] != null)
    {
        lock (_dataSyncObjects[defaultIndex])
        {
            if (_dataToBeSent[defaultIndex] != null)
            {
                _dataToBeSent[defaultIndex].Dispose();
                _dataToBeSent[defaultIndex] = null;
            }
        }
    }
}

You're right; I'll make the change.

fixed

Sorry for the late review. I am not familiar with the error path but if _dataToBeSent array can contain null entries then it seems like so can _dataSyncObjects (_dataSyncObjects[promptResponseIndex] == null ?).

It seems like these arrays should be created in the PrioritySendDataCollection constructor...

Paul Higinbotham (@PaulHigin): _dataSyncObjects is populated when it is created. The entries are not null if the array is not null.
As far as when it is created, it appears the intent was to create both arrays at the same point. I saw no strong reason to change it.

But the _dataSyncObjects array is filled within a loop:

for (int i = 0; i < names.Length; i++)
    {
        _dataToBeSent[i] = new SerializedDataStream(_fragmentor.FragmentSize);
        _dataSyncObjects[i] = new object();
    }

If an exception is thrown within the loop then it is possible an array element is null. Is this not the error path we are worried about?

SerializedDataStream's ctor does not currently throw.

Note that it should (it relies on Dbg.Assert) and I opened an issue for this: #5800