Почему синхронизация по параметру метода "опасна"

Ответ 1

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

Обновить

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

Теперь, если вы переместите метод fireFinalCallback в свой класс ShortCircuit вы можете гарантировать, что обратный вызов не будет срабатывать одновременно. В противном случае вы должны иметь это в виду при вызове методов этого класса.

Ответ 2

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

synchronized (s)
{
    if (!s.isFinalCallbackFired())
    {
        s.doFire();
    }
}

То есть: сначала проверяется, возвращает ли isFinalCallbackFired false, и если это так, выполняется что-то (скрытое), что, вероятно, приводит к isFinalCallbackFired состояния isFinalCallbackFired в значение true.

Таким образом, я предполагаю, что цель помещения оператора if в synchronized блок состояла в том, чтобы убедиться, что doFire всегда вызывается ровно один раз.

И действительно, на этом этапе синхронизация может быть оправдана. Более конкретно, и немного упрощенно:

Что может быть гарантировано:

Когда два потока исполняют метод fireFinalCallback с одним и тем же параметром ShortCircuit, synchronized блок гарантирует, что только один поток может одновременно проверять состояние isFinalCallbackFired и (если он имеет значение false) вызывать метод doFire. Таким образом, гарантируется, что doFire будет вызываться только один раз.

Что не может быть гарантировано:

Когда один поток выполняет метод fireFinalCallback, а другой поток выполняет какие-либо операции с объектом ShortCircuit (например, вызов doFire), это может привести к несогласованному состоянию. В частности, если другой поток также делает

if (!s.isFinalCallbackFired())
{
    s.doFire();
}

но без синхронизации на объекте doFire может быть вызван дважды.

Ниже приведен MCVE, который иллюстрирует эффект:

public class SynchronizeOnParameter
{
    public static void main(String[] args)
    {
        System.out.println("Running test without synchronization:");
        runWithoutSync();
        System.out.println();

        System.out.println("Running test with synchronization:");
        runWithSync();
        System.out.println();

        System.out.println("Running test with wrong synchronization:");
        runWithSyncWrong();
        System.out.println();

    }

    private static void runWithoutSync()
    {
        ShortCircuit s = new ShortCircuit();
        new Thread(() -> fireFinalCallbackWithoutSync(s)).start();
        pause(250);
        new Thread(() -> fireFinalCallbackWithoutSync(s)).start();
        pause(1000);
    }

    private static void runWithSync()
    {
        ShortCircuit s = new ShortCircuit();
        new Thread(() -> fireFinalCallbackWithSync(s)).start();
        pause(250);
        new Thread(() -> fireFinalCallbackWithSync(s)).start();
        pause(1000);
    }

    private static void runWithSyncWrong()
    {
        ShortCircuit s = new ShortCircuit();
        new Thread(() -> fireFinalCallbackWithSync(s)).start();

        if (!s.isFinalCallbackFired())
        {
            s.doFire();
        }
    }



    private static void fireFinalCallbackWithoutSync(ShortCircuit s)
    {
        if (!s.isFinalCallbackFired())
        {
            s.doFire();
        }
    }

    private static void fireFinalCallbackWithSync(ShortCircuit s)
    {
        synchronized (s)
        {
            if (!s.isFinalCallbackFired())
            {
                s.doFire();
            }
        }
    }

    static class ShortCircuit
    {
        private boolean fired = false;

        boolean isFinalCallbackFired()
        {
            return fired;
        }

        void doFire()
        {
            System.out.println("Calling doFire");
            pause(500);
            fired = true;
        }
    }

    private static void pause(long ms)
    {
        try
        {
            Thread.sleep(ms);
        }
        catch (InterruptedException e)
        {
            e.printStackTrace();
        }
    }

}

Выход

Running test without synchronization:
Calling doFire
Calling doFire

Running test with synchronization:
Calling doFire

Running test with wrong synchronization:
Calling doFire
Calling doFire

Таким образом, synchonized блок гарантирует, что метод doFire вызывается только один раз. Но это работает, только если все модификации выполняются только в методе fureFinalCallback. Если объект изменяется в другом месте, без synchronized блока, метод doFire может быть вызван дважды.

(Я хотел бы предложить решение для этого, но без подробностей о классе ShortCircuit и оставшихся классах и процессах, можно было бы дать лишь смутный намек, чтобы взглянуть на пакет java.util.concurrent и его подпакеты: Locks и условия могут быть жизнеспособным путем, но вы должны это выяснить...)