1. Java / Говнокод #16590

    +76

    1. 1
    2. 2
    3. 3
    4. 4
    5. 5
    private Boolean active = false;
...
synchronized (active) {
...
}

    Чудо синхронизации. Блокируется раз и навсегда.

    Запостил: borka, 25 Августа 2014

    Комментарии (16) RSS

    • ava TauSigma 25.08.2014 21:27 # 0

      А в чём отличие явовского synchronized от .net' овского lock?
      Судя по мануалу:
      http://docs.oracle.com/javase/tutorial/essential/concurrency/locksync.html
      Особой разницы нет.
      Ответить

    • ava kegdan 25.08.2014 21:48 # 0

      А почему раз и навсегда? (хлоп-хлоп глазами)
      Ответить

      • ava borka 25.08.2014 22:24 # +5

        Потому что под synchronized долгоживущий код, а Boolean.FALSE / Boolean.TRUE в единственном экземпляре на всю JVM.
        Должно быть что-то типа synchronized(this).
        Ответить

        • ava 3.14159265 26.08.2014 00:09 # 0

          >synchronized(this)
          Тоже считается плохим тоном - если совсем по-правильному.
          Имеем утечку приватного мьютекса, пользователь может синхронизироваться на данном объекте и сломать абстракцию. Потому в тех же коллекциях this не используют.
          Я бы предпочёл private final AtomicBoolean - если код многопоточный вероятно это лучше чем мутабельная ссылка, на которой синхронизируются.
          или private Boolean active = new Boolean(false);
          Ответить

          • ava borka 26.08.2014 00:41 # 0

            Прятать/не прятать - дело хозяйское. Вообще неплохо иметь дополнительный конструктор в который передается внешний объект синхронизации. Например можно заводить один лок на несколько объектов и не боятся дедлока при различных путях вызовов между этими объектами.
            Ответить

            • ava 3.14159265 27.08.2014 18:23 # 0

              >Вообще неплохо иметь дополнительный конструктор в который передается внешний объект синхронизации.
              >Например можно заводить один лок на несколько объектов и не боятся дедлока
              Согласен. Вот в каллекциях засинхронизировали всё внутри, а итераторы забыли.
              А конструкторы полезные с этим самым мьютексом package-private.
              Ответить

              • ava bormand 27.08.2014 19:02 # 0

                А в чем сакральный смысл делать две коллекции с одним мутексом?

                Дедлока тут и так не может быть, ибо мутекс захвачен только на время работы метода. И ни один метод, емнип, управление из-под лока не отдает (ну разве что в какой-нибудь equals).

                Атомарности это не добавит, ибо мутекс захватывается только на время работы метода коллекции. И если я вызову джва метода - между ними всё равно будет дырка.

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

                • ava 3.14159265 27.08.2014 19:33 # 0

                  >А в чем сакральный смысл делать две коллекции с одним мутексом?
                  Мапа - раз.
                  Её view KeySet - два.
                  Её view valueCollection - три.
                  Итераторы на keys, values, entries.

                  Видишь ли view мапы может менять её наравне с обычными методами, и каждый из них необходимо заблочить одним и тем же мьютексом.
                  Даже в итераторе есть мутирующий remove (что лично по-моим соображениям - ошибка в проектировании, обычный итератор должен быть ronly, т.к. 95% юзкейсов - обычный обход).
                  Ответить

                  • ava bormand 27.08.2014 20:01 # 0

                    > view мапы может менять её наравне с обычными методами, и каждый из них необходимо заблочить одним и тем же мьютексом.
                    Так эти вьюхи же получают этот мутекс в конструкторе, когда мапа их запиливает. Разве нет?

                    > итераторе
                    Кстати, а как работает итератор у этих недосинхронизированных коллекциях?

                    P.S. Нашел - никак не работает. Нужно самому лочиться об мапу :( Поэтому итератор тут не навредит - его за мутексом юзать вообще нельзя.

                    P.P.S. И я так и не понял, от чего спасёт возможность передачи внешнего мутекса недосинхронизированным коллекциям.
                    Ответить

                    • ava 3.14159265 27.08.2014 23:17 # 0

                      >>Так эти вьюхи же получают этот мутекс в конструкторе, когда мапа их запиливает. Разве нет?
                      Вьюхи это ответ на вопрос:
                      >А в чем сакральный смысл делать две коллекции с одним мутексом?
                      Ответить

                      • ava bormand 28.08.2014 06:31 # 0

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

                        А для независимых особого смысла в передаче мутекса я не вижу.
                        Ответить

                  • ava bormand 27.08.2014 20:03 # 0

                    > обычный итератор должен быть ronly, т.к. 95% юзкейсов - обычный обход
                    Ну вот, кстати, в тех же крестах правильно сделали - итератор не может удалять элементы. Зато можно передать этот итератор соотв. методу коллекции, и он выпилит элемент (или несколько).
                    Ответить

        • ava kegdan 27.08.2014 19:46 # 0

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

    • ava Lennis 26.08.2014 17:38 # +6

      "Пофиксил"

      synchronized(new Object()){
...
}
      Ответить

    • ava guest 26.08.2014 21:41 # −1

      А-ах, как умно...
      Автор, утопись в отхожем мисце!
      Ответить

    Добавить комментарий