Почему плохой способ заблокировать объект, который мы собираемся изменить?

Почему плохой практикой является использование блокировки, как в следующем коде, я предполагаю, что это плохая практика, основанная на ответах в этом SO-вопросе здесь

private void DoSomethingUseLess()
{
    List<IProduct> otherProductList = new List<IProduct>();
    Parallel.ForEach(myOriginalProductList, product =>
        {
           //Some code here removed for brevity
           //Some more code here :)
            lock (otherProductList)
            {
                otherProductList.Add((IProduct)product.Clone());
            }
        });
}

В ответах на там упоминается, что это плохая практика, но они не говорят, почему

Примечание: Пожалуйста, проигнорируйте полезность кода, это просто, например, цель, и я знаю, что это совсем не полезно

Ответ 1

Из справочника языка С# здесь:

В общем, избегайте блокировки открытого типа или экземпляров, находящихся вне вашего кода. Общие конструкции lock (this), lock (typeof (MyType)) и lock ("myLock") нарушают это правило:

lock (this) является проблемой, если к экземпляру можно получить доступ публично.

lock (typeof (MyType)) является проблемой, если MyType является общедоступным.

lock("myLock") является проблемой, поскольку любой другой код в процессе используя ту же строку, будет иметь один и тот же замок.

Лучшей практикой является определение частного объекта для блокировки или частного статическая переменная объекта для защиты данных, общих для всех экземпляров.

В вашем случае я прочитал бы приведенное выше руководство, предполагая, что блокировка коллекции, которую вы будете модифицировать, является плохой практикой. Например, если вы написали этот код:

lock (otherProductList) 
{
    otherProductList = new List<IProduct>(); 
}

... тогда ваш замок будет бесполезен. По этим причинам рекомендуется использовать специальную переменную object для блокировки.

Обратите внимание, что это не означает, что ваше приложение будет ломаться, если вы используете код, который вы опубликовали. "Лучшие практики" обычно определяются для обеспечения легко повторяющихся шаблонов, которые более технически устойчивы. То есть, если вы будете следовать лучшей практике и иметь выделенный "объект блокировки", вы вряд ли когда-либо напишете сломанный lock код; если вы не будете следовать лучшей практике, то, возможно, один раз в сотне, вас укусит легко устраняемая проблема.

Дополнительно (и в более общем плане) код, написанный с использованием лучших практик, обычно более легко модифицируется, потому что вы можете быть менее осторожным в отношении неожиданных побочных эффектов.

Ответ 2

Это может быть не очень хорошо, потому что, если кто-то использует ту же ссылку на объект, что и lock, у вас может быть тупик. Если есть шанс, что ваш заблокированный объект доступен вне вашего собственного кода, тогда кто-то может сломать ваш код.

Представьте следующий пример, основанный на вашем коде:

namespace ClassLibrary1
{
    public class Foo : IProduct
    {
    }

    public interface IProduct
    {
    }

    public class MyClass
    {
        public List<IProduct> myOriginalProductList = new List<IProduct> { new Foo(), new Foo() };

        public void Test(Action<IEnumerable<IProduct>> handler)
        {
            List<IProduct> otherProductList = new List<IProduct> { new Foo(), new Foo() };
            Parallel.ForEach(myOriginalProductList, product =>
            {
                lock (otherProductList)
                {
                    if (handler != null)
                    {
                        handler(otherProductList);
                    }

                    otherProductList.Add(product);
                }
            });
        }
    }
}

Теперь вы компилируете свою библиотеку, отправляете ее клиенту, и этот клиент пишет в своем коде:

public class Program
{
    private static void Main(string[] args)
    {
        new MyClass().Test(z => SomeMethod(z));
    }

    private static void SomeMethod(IEnumerable<IProduct> myReference)
    {
        Parallel.ForEach(myReference, item =>
        {
            lock (myReference)
            {
                // Some stuff here
            }
        });
    }
}

Тогда для вашего клиента может быть приятный отладочный тупик, каждый из двух используемых потоков, ожидающих, что экземпляр otherProductList больше не будет заблокирован.

Я согласен, этот сценарий вряд ли случится, но он иллюстрирует, что , если ваша заблокированная ссылка видна в фрагменте кода, который у вас нет, любым возможным способом, тогда есть возможность для окончательного кода сломаться.