Pull to refresh

Comments 66

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

Эээ?


using(Stream s = GetRequestStream())
{
    using(XmlReader r = XmlReader.Create(s))
    {
      //...
    }
}

Собственно, написано же: "DO allow the Dispose(bool) method to be called more than once. [...] Users expect that a call to Dispose will not raise an exception."

В этом примере XmlReader разрушает то, что создал не он сам. При этом внешний код, зная что объект разрушен ставит using, создавая дополнительный try… finally, который дополнительно замедляет приложение. Выглядит красиво, удобно, но не считаю это хорошей архитектурой.
Выглядит красиво, удобно, но не считаю это хорошей архитектурой.

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


При этом внешний код, зная что объект разрушен ставит using, создавая дополнительный try… finally, который дополнительно замедляет приложение.

Есть конкретный бенчмарк, который показывает, насколько это медленнее?


Понимаете, если вы нарушите это соглашение (allow for multiple disposal), вы нарушите контракт, который предлагают разработчики фреймворка, тем самым создав бесконечное количество WTF. Не надо так.

Это типовое решение.

Это не типовое решение, а решение принятое разработчиками платформы. Возможно это был выбор меньшего зла. Но это не означает, что мы теперь везде должны делать тоже самое.
Если это был выбор меньшего зла, то зачем же нам теперь выбирать большее зло? Да и не важно уже что было большим злом, а что меньшим. На данный момент, возможность повторного вызова Dispose — это поведение, ожидаемое большинством .Net разработчиков. Именно это определяющий фактор. Основная причина чтобы «делать то же самое».
Впрочем, вас никто не заставляет повторно использовать Dispose в своём коде. Речь лишь о том, чтобы позволить так делать другим, когда вы проектируете IDisposable класс. И пусть это позволит кому-то писать менее оптимальный код. Ведь не заставит, а лишь позволит.
Если это был выбор меньшего зла, то зачем же нам теперь выбирать большее зло?

Чтобы не было недопонимания поясню. Я вижу два вопроса: 1) должен ли посторонний код диспозить объект, который он не создавал. 2) нужно ли запрещать повторный dispose.

И мой комментарий относился именно к первому вопросу, поскольку веткой выше обсуждался XmlReader. Я имел ввиду, что наименьшее зло — это то, что reader|writer диспозят stream. Однако, в других ситуациях такое решение может быть не оправдано. Хотя лично я и в части стримов, такое решение не оправдываю.

По поводу исключений в Dispose мне нечего сказать. Да, упадём в рантайме при повторном вызове. Но, не исключено, что повторный вызов говорит, о неверной работе программы. Исключение гораздо быстрее обнаруживается и отлаживается, чем скрытые баги. Думаю, это хотел донести автор.
должен ли посторонний код диспозить объект, который он не создавал.

Это извечная проблема. Нет четкого разделения между передачей во временное пользование, и передачей во владение. В первом случае диспозить нельзя, во втором — нужно. Тут не помешала бы move семантика. Что-то вроде плюсового unique_ptr.

Это как раз типовое решение в том смысле, что в .net-экосистеме (в BCL, в скачиваемых пакетах, в разработках) так уже принято. Вы можете так не делать — в том смысле, что вы можете никогда не захватывать контроль над переданным вам ресурсом — но вы не можете ожидать, что никто другой не будет так делать.

Как принято? Вызывать Dispose откуда придётся? Нет, в .net-экосистеме так не принято. Очень много методов и типов, которые так не делают. Так делают лишь упомянутый XmlReder и другие reader-ы/writer-ы. Ну может ещё пара-тройка мест.
Как принято?

Закрывать переданный тебе ресурс, если известно, что ты потребил его полностью. Early release, вот это все.


Равно как и все равно закрывать ресурс, даже если ты его кому-то отдал, в момент, когда он теряет актуальность (например, поток запроса при окончании обработки запроса).


Так делают лишь упомянутый XmlReder и другие reader-ы/writer-ы.

Это уже достаточно много, чтобы понимать, что поток может быть закрыт кем угодно в любой момент.

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

Скорее я бы назвал это типовой ошибкой. В данном случае все получилось замечательно, но добавляем одну маленькую деталь — и все валится :-)


public Message Foo() 
{
    using(Stream s = GetSomeStream())
    {
        using(XmlReader r = XmlReader.Create(s))
        {
            return Message.CreateMessage(r, int.MaxValue, MessageVersion.Soap12);
        }
    }
}

Что характерно, если бы XmlReader не закрывал стрим внутри, ничего бы не изменилось.

Да нет, изменилось бы. Класс Message в его текущем виде просто не смог бы появиться: его реализация завязана на тот факт, что закрытие XmlReader освободит все связанные с ним ресурсы.

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


(в Rx вот сделали даже оператор специальный, чтобы привязывать время жизни одной последовательности к другой)

Например, ThreadAbortException, который не перехватывается ни catch ни finally.

Перехватывается
ThreadAbortException is a special exception that can be caught, but it will automatically be raised again at the end of the catch block. When this exception is raised, the runtime executes all the finally blocks before ending the thread.
Хороший пример: а вы знали что IEnumerable<T> тянет за собой IDisposable?

Не тянет (.net 4). Другое дело, что класс, реализующий IEnumerable, может одновременно реализовывать IDisposable, и foreach в C# это учтет.

Необходимое уточнение: IEnumerator<T> действительно требует IDisposable (в отличие, кстати, от IEnumerator).

Кажется, вы забыли упомянуть о еще одном важном достоинстве SafeHandle. Дело в том, что какой-нибудь ThreadAbortException может прилететь после вызова CreateFile — но еще до установки значения поля _handle. Без использования SafeHandle это приведет к редкой но очень неприятной утечке ресурса.

Да, спасибо, что напомнили :) Дополню в ближайшее время
Dispose кидает исключения, дожили.
Он должен работать как часы в любом положении, повторный вызов — частый кейс.
Называете статью полным описанием, но я так и не увидел хорошей рекомендации, как НАДО делать. А между тем, есть вполне себе отличный вариант на SO — ru.stackoverflow.com/a/486697/196257
Где вы нашли рекомендацию кидать исключения? Таких ситуаций допускать нельзя. Их два: оба описаны. Как надо, описано. Для Managed шаблон готовый, для unmanaged — тоже в целом описано. SafeHandle использовать вместо велосипедов. Либо делать свой, но наследовать от CriticalFinalizerObject + Dispose + finalizer. Вся схема описана в разделе Два уровня Disposable Design Principle.

Но согласен, стоит выделить итоги отдельно.
Где вы нашли рекомендацию кидать исключения?

А что делает ваш CheckDispose который вы вызываете внутрях Dispose?

Это единственное и правильное исключение. Оно говорит о том что тот кто пытается разрушить объект не совсем в курсе что сам делает и пытается освободить освобожденные ресурсы. Я в курсе что MS много где разрешает. Это не корректно. То что MS что-то сделала не значит что это — панацея.
Второй вызов у вас стабильно кидает исключение, я вроде довольно понятно это написал. Абсолютно весь коробочный dotNet позволяет делать повторный вызов, ваша реализация — нет. Извините, но это глупость. Вести себя надо как коробка, просто потому что коробку используют все.

Отдали вы в сторонюю либу свою реализацию IDisposable, а она падать начала на вашем исключении. Будете либу патчить? Удачи, время потрачено зря.

ПС: «Два уровня Disposable Design Principle» неудобный паттерн, т.к. смешивать управляемые и неуправляемые — неудобно и смешивает обычно уровни абстракций. Да, он популярный, нет, его не надо вставлять в статью, ибо он выглядит рекомендацией.

Я отстаиваю и буду до упора продолжать отстаивать позицию исключений при повторном вызове Dispose(). Эта "возможность" введена чтобы без лишнего геморроя разрушить граф объектов, например, начав с любого узла. Т.е. алгоритм криво спроектирован и позволяет себе повторные вызовы на одинаковых объектах. По моему скромному мнению это — защита от криворукости программистов с полным пониманием того что IDisposable, который они сделали для некоторых ситуаций требует сноровки чтобы все разрулить без повторных вызовов.


Отдали вы в сторонюю либу свою реализацию IDisposable, а она падать начала на вашем исключении. Будете либу патчить? Удачи, время потрачено зря.

Значит я криворук и отдал баг на продакшн, а QA фигово оттестировали.


Назовите мне, пожалуйста, реальный кейс где без повторного вызова — ну никак? :)


«Два уровня Disposable Design Principle» неудобный паттерн, т.к. смешивать управляемые и неуправляемые — неудобно и смешивает обычно уровни абстракций

Паттерн ничего не смешивает, а наоборот — разделяет ответственность. Level 0 = это, например, SafeHandle. Он заведует только IntPtr и все. Никаких управляемых ресурсов. Либо какой-то свой класс, унаследованный от CriticalFinalizerObject и инкапсулирующий, например IntPtr и другой SafeHandle. Тогда делается икремент счетчика ссылок чтобы порядок финализации задать. Level 1 = разрушение только управляемых ресурсов. Полное разделение ответственности

Кроме кейса от lair — есть ещё куча либ, которые в конструктор принимают стрим… и всё. Т.е. они не сообщают никак, закроют его за собой или нет.

Что должен делать в такой ситуации разработчик? Я предпочитаю закрывать стрим, потому как он мной создан, мной и закрыт. А потом оказывается, что либа считает принятый в конструктор стрим — своим, закрывает сама. Только с вашим вариантом об этом узнать придётся в рантайме, поймав исключение, а с коробочным вариантом — не придётся даже заморачиваться. Будет просто работать. Как собственно и должен делать хороший код.
+ lair Хорошо, убедили. Мне, видимо, повезло со сторонним кодом ). Но это я скорее запишу в минусы шаблона чем в плюсы. Все должно быть однозначно на каждом этапе работы приложения.
Я бы не сказал что пример по ссылке полностью корректен. Например, KeepAlive(this) в ctor — зачем? ). Множественный вызов Dispose() — зачем? Является ли множественный запрос SQL к серверу на всякий случай нормой и если нет, почему множественные попытки разрушить объект — норма? :)
Однажды такое приложение завалится с OutOfMemoryException
Но стоит заметить, что вовсе не обязательно приложение падает с такого исключения, см. пример на скорую

Мне кажется, что тут в принципе стоит заметить, как себя ведёт приложение при различных исключениях:
  • StackOverflowException — приложение падает в нуль и без кетчей. Чтобы избежать такого — можно чекать стек через RuntimeHelpers.EnsureSufficientExecutionStack
  • ThreadAbortException — ловится в catch, но пробрасывается выше
  • OutOfMemoryException — ловится в кетч, если по факту памяти уже достаточно, то приложение продолжает работу
  • ExecutionEngineException — что-то не так в самом CLR пошло

Подробнее см. Exceptional Exceptions in .NET
Спасибо за уточнение, тип исключения был выбран в качестве примера. Дополню всеми типами.
Посмотрел повнимательнее, мой пример корректен, т.к. речь именно о нехватке памяти )
Очень хорошая статья, написанная легким языком. Надеюсь допилите книгу.
поддерживаю, и добавлю: надеюсь, допилите книгу и учтете комментарии, все-таки при таком амбициозном подходе важно сохранить объективность (которая не только в «как надо» но и в «как делают»).
Мало того, я еще и добавил в конец раздела всех кто внес вклад :)
Может быть Вам нужны еще руки / головы для столь доблестного труда?

Спасибо автору за статью и всем за коменты, они не менее интересны.

Хотел еще добавить. Вряд ли мне придется применить на практике знания полученные из этой статьи и ее обсуждения в ближайшее время, но все это очень познавательно и для себя я пришел к выводу, что оба варианта имеют права на жизнь.
Если делать API для узкого круга лиц и/или для проекта в котором критична скорость освобождения ресурсов, то вариант автора намного более предпочтителен.
Если же делать API «для всех», то однозначно только вариант допускающий множественные Dispose, т.к. это уже давно устоявшаяся практика для «всех» и в 1-ую все должно быть надежно и интуитивно понятно.
Но с другой стороны C# ведь в принципе не предназначен для систем реального времени и возможно концепция автора просто не имеет смысла, а в случаях когда критична скорость освобождения ресурсов следует изначально выбирать C++ и т.п. и полностью контролировать ситуацию. Буду рад объективной критике.
В целом я отошел от исключения, добавив ремарку что это минус подхода. Скорее говорит о некомпетентности тех кто использует API и о способах защиты от этой некомпетентности. И то что так стало принято повсеместно, наверное, заставляет и нас поступать также.
Если делать API для узкого круга лиц и/или для проекта в котором критична скорость освобождения ресурсов, то вариант автора намного более предпочтителен.

Почему?

Что именно почему? Все ведь расписано в моем посте. Если в каком-то проекте критична скорость освобождения ресурсов, то вариант автора оказывается более предпочтителен (в теории). А на практике, я считаю, реальной пользы от этого не будет, т.к. сборщик мусора приезжает когда захочет и прироста производительности от освобождения ресурсов в «правильном» порядке без дублирования не будет заметно.

Именно что почему "вариант автора оказывается более предпочтителен, если критична скорость освобождения ресурсов". Какая именно особенность решения в посте повышает скорость освобождения?

Для начала, что такое "скорость освобождения ресурсов"? Время, затраченное на вызов Dispose? Или время от создания объекта до момента, когда его ресурсы освобождены (успешно вызван Dispose)?

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

Для меня важно лишь определиться имеет ли смысл использовать данный паттерн.
В общих случаях, я думаю всем очевидно, что нет. Т.к. страдает простота, красота, читабельность и т.д. кода, увеличивается объем инфраструктурного кода, может возникнуть куча проблем с проектированием и рефакторингом из ничего и т.д. И в итоге можно получить проектирование ради проектирования…

Как я писал выше возможно это может иметь смысл в каких-то частных случаях, как вариант оптимизации, но лично я не вижу каких-то явных плюсов в том, что ВОЗМОЖНО сократится количество вызовов Dispose такой ценой.

Я просто не могу представить задачу или ситуацию когда использование этого паттерна в контексте .NET будет оправдано. Автор в статье упоминал про граф объектов… В частном случае, если классы этих объектов описывают относительно примитивные сущности, целесообразным будет использование структур, а не классов со всеми вытекающими плюсами типов значений над ссылочными и это даст ощутимую оптимизацию быстродействия и использования памяти, если выделять память сразу для больших групп таких объектов.

Возможно не совсем верная аналогия, но это можно сравнить с авто с АКПП и МКПП. И использование этого паттерна это попытка ехать на АКПП как на МКПП. Но если критичны возможности МКПП, не лучше ли изначально выбрать МКПП.
Ведь основные преимущества C# это скорость разработки и высокая сложность отстрелить себе ноги, а в данном случае мы получается пытаемся идти против этих основных концепций.

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

Так оно не зависит от того, что внутри Dispose.


Для меня важно лишь определиться имеет ли смысл использовать данный паттерн.

Какой именно?

Так оно не зависит от того, что внутри Dispose.

А я разве утверждал обратное? В том то собственно и вопрос, что мы по сути пытаемся оптимизировать количество вызовов методов которые по сути ничего не делают и сразу вызывают return.
Какой именно?

Вариант автора статьи. Я ведь очень подробно изложил свои выводы.
А я разве утверждал обратное?

Вы утверждали, что "Если делать API для узкого круга лиц и/или для проекта в котором критична скорость освобождения ресурсов, то вариант автора намного более предпочтителен.". Хотя в реальности вариант автора статьи не окажет влияния на скорость освобождения ресурсов.


Вариант автора статьи.

В том-то и дело, что вариант автора статьи — это не паттерн.


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

В том-то и дело, что вариант автора статьи — это не паттерн.

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

Полностью согласен. Но для чего и зачем это в принципе может быть нужно?
Но для чего и зачем это в принципе может быть нужно?

Beats me. Это вопрос к тем, кто утверждает, что многократный Dispose — признак плохой архитектуры.

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

Я думаю в этом кроется ваше желание кидать эксепшн при повторном вызове Dispose. У меня другое понимание: IDisposable — способ узнать, что конкретный тип требует освобождения захваченных ресурсов. Разрушение — это только одно из следствий вызова метода (на самое главное). Много где по тексту я бы заменил разрушение на освобождение.
Насколько я понял, основная цель этих эксепшенов построить приложение в котором не будет повторных Dispose. Но я так и не решил для себя однозначно, имеет ли это смысл.
Вы меня навели на правильную мысль. Lingvo для Disposable подсказывает среди прочего вариант: выбрасываемый (после употребления). В русскоязычном IT сленге, наверное, «освобожденный» тут мало подходит. Выброшенный, использованный — не говорят и любого закидают тухлыми яйцами если так кто-то скажет :) Тут скорее уже стоит подбирать по смыслу. Кстати если переводить Dispose() как «прекратить использовать», то невыброс AlreadyDisposedException() — правильно и ставит все на свои места. Но перевод не привычен для нашего языка и «разрушение», как мне кажется, меньшее из зол.
Думаю, что называть это некомпетентностью все же не верно. Ведь освобождение ресурсов в «правильном» порядке без повторов по сути ничего не дает. А отсутствие необходимости думать об этом дает довольно много. В 1-ую очередь повышает скорость разработки и надежность.

Очень хорошая фундаментальная статья.
Есть пара-тройка замечаний:


  1. Поведение при вызове уже освобожденного (разрушенного) объекта логично считать неопределенным.
    Это полный аналог проверок на null аргументов и результатов методов. Они полезны при отладке, но в релизе могут быть удалены по соображениям производительности: все равно к удаленному объекту без ошибок в коде никто не обращается. Добавление таких проверок вручную нецелесообразно (они чисто шаблонные), тут подходящие условия для применения АОП, например, специального плагина для Fody.
  2. Поведение при двойном Dispose также логично считать непределенным из тех же соображений. Только тут есть два нюанса: передача IDisposable тому, кто не является владельцем (включая наследование интерфейсов от IDisposable) — плохой дизайн и этот плохой дизайн, к сожалению, широко распространен. В результате на практике повторный вызов Dispose лучше всего игнорировать (писать в лог в отладочной версии).
  3. Жесткий порядок освобождения и различие level-1 и level-0 для их владельца скорее вредно чем бесполезно. При отсутсвии ошибок дизайна (у каждого IDisposable ровно один владелец) все и так сработает хорошо, при наличии — все и так будет плохо.
  4. Критика паттерна на мой взгляд излишне суровая — возможно, лучше сосредоточиться на антипаттернах, связанных с неверным пониманием низкоуровневой природы IDisposable. Классический пример — требование передачи стримов и ридеров, реализующих IDisposable. Это сразу создает двух владельцев со всеми вытекающими.
  5. Главу про lifetime логично объединить с этой или поставить в книге сразу за ней.

Еще раз спасибо за глубину и точность сведений в статье.

Only those users with full accounts are able to leave comments. Log in, please.

Information

Founded
Location
Россия
Website
clrium.ru
Employees
1 employee (me only)
Registered
Representative
Stanislav Sidristij

Habr blog