вторник, 14 октября 2014 г.

Коротко. О "частично инициализированных объектах" (или опять о фабриках)

По мотивам:

Коротко. О конструкторах
Депрессия. Или превратности hResult и прочих ErrorCode
Коротко. И ещё о фабриках
Коротко. Ещё о фабриках
Коротко. О фабриках
Коротко. "Почему нужны тесты"
Фабричный метод

Сегодня таки (надеюсь) нашёл ошибку, которая "не давала жить" многие годы.

И следы которой мы наблюдали во всяких разных логах.

Но мы долго не могли её "опространствить".

Но сегодня "я поймал её за хвост". Под отладчиком.

И как ни банально -дело во всё тех же "частично инициализированных объектах".

Вот примерно так (код естественно - по-максимуму упрощаю):

construstor TStorageHeader.Create(aPosition: Int64);
begin
 Assert(aPosition >= 0);
 inherited Create;
 FillChar(f_Data, SizeOf(f_Data), 0); // - инициализируем данные
 f_Position := aPosition;
 LockFile(f_Position, SizeOf(f_Data)); // - тут блокируем регион файла и там может ПОДНИМАТЬСЯ ИСКЛЮЧЕНИЕ - LOCK_VIOLATION
 LoadData(f_Position, f_Data, SizeOf(f_Data)); // - тут читаем данные 
end;

destructor TStorageHeader.Destroy;
begin
 SaveData(f_Position, f_Data, SizeOf(f_Data)); // - тут записываем данные 
 UnLockFile(f_Position, SizeOf(f_Data)); // - тут разблокируем регион файла
 inherited;
end;

В чём тут проблема?

А вот в чём:

var
 l_H : TStorageHeader;
begin
 l_H := TStorageHeader.Create(aSomePosition);
 try
  ...
 finally
  FreeAndNil(l_H);
 end;//try..finally
end;

- если в конструкторе НЕТ ИСКЛЮЧЕНИЯ, то всё хорошо.

А вот если в конструкторе ЕСТЬ ИСКЛЮЧЕНИЕ, то АВТОМАТОМ вызовется деструктор.

На ЧАСТИЧНО ИНИЦИАЛИЗИРОВАННОМ объекте.

Что произойдёт? Если исключение было возбуждено в LockFile то код LoadData - не пройдёт.

Но! Деструктор - вызовется (читаем документацию).

Цитирую:

"Destructors called because of exceptions thrown from constructors"

И что произойдёт?

А то что в коде SaveData - сохранится НЕ ТО ЧТО СЧИТАЛИ, а то что инициализировали при помощи FillChar.

Проблема понятна?

Она - банальна. Но однако "жила многие годы". Почему? Потому что вероятность того что из LockFile "вылетит исключение" - была крайне низка. Потому что там блокировка происходит с учётом тайм-аута. Который для большинства операций - достаточен. Чтобы не летело исключение.

А более того - у нас был код (выше), который синхронизировал доступ к ресурсам на уровне "бизнес-логики".

Но!

Эта синхронизация негативно складывалась на производительности. То самое "бутылочное горлышко".

Клиенты "ждали друг друга" и не лезли к "общему ресурсу". Поэтому дело до LockFile который может "возбудить исключение" - не доходило. Просто - "все выстраивались в очередь" и работали последовательно, а не параллельно.

(Кстати вот что стоит почитать - Э. ДЕЙКСТРА ВЗАИМОДЕЙСТВИЕ ПОСЛЕДОВАТЕЛЬНЫХ ПРОЦЕССОВ. Хотя это "к делу не относится")

Но!

Мы разрулили это "бутылочное горлышко" и клиенты начали работать "по-настоящему параллельно и независимо".

И вероятность  получить исключение и "проблемный код" - резко возросла.

А на автоматических тестах - тем более. Т.к. они работают без задержек и пауз, свойственных людям.

И ошибка стала появляться более стабильно и очень часто.

Особенно если запускать тесты под отладчиком и вставать на break-point'ах там "где надо". Эмулируя "задержки сети".

Что делать?

Ну в общем - "всё просто". Можно сделать например так:

construstor TStorageHeader.Create(aPosition: Int64);
begin
 Assert(aPosition >= 0);
 inherited Create;
 f_DataIsLoaded := false; // - для "надёжности"... а на самом деле, чтобы ПОСТУЛИРОВАТЬ наши намерения
 FillChar(f_Data, SizeOf(f_Data), 0); // - инициализируем данные
 f_Position := aPosition;
 LockFile(f_Position, SizeOf(f_Data)); // - тут блокируем регион файла и там может ПОДНИМАТЬСЯ ИСКЛЮЧЕНИЕ - LOCK_VIOLATION
 LoadData(f_Position, f_Data, SizeOf(f_Data)); // - тут читаем данные 
 f_DataIsLoaded := true; // - тут постулируем, что таки зачитали данные
end;

destructor TStorageHeader.Destroy;
begin
 if f_DataIsLoaded then // - пишем только то, что СЧИТАЛИ, а не "мусор"
  SaveData(f_Position, f_Data, SizeOf(f_Data)); // - тут записываем данные 
 UnLockFile(f_Position, SizeOf(f_Data)); // - тут разблокируем регион файла
 inherited;
end;

Ну или так:

construstor TStorageHeader.Create(aPosition: Int64);
begin
 Assert(aPosition >= 0);
 inherited Create;
 FillChar(f_Data, SizeOf(f_Data), 0); // - инициализируем данные
 f_Position := aPosition;
 LockFile(f_Position, SizeOf(f_Data)); // - тут блокируем регион файла и там может ПОДНИМАТЬСЯ ИСКЛЮЧЕНИЕ - LOCK_VIOLATION
 // - и там же "взводим флаг"- IsLocked
 LoadData(f_Position, f_Data, SizeOf(f_Data)); // - тут читаем данные 
end;

destructor TStorageHeader.Destroy;
begin
 if Self.IsLocked then // - пишем ТОЛЬКО если "заблокировали регион", а "стало быть" - и считали его
  SaveData(f_Position, f_Data, SizeOf(f_Data)); // - тут записываем данные 
 UnLockFile(f_Position, SizeOf(f_Data)); // - тут разблокируем регион файла
 inherited;
end;

Но "на мой вкус" - лучше сделать "фабричный метод".

Примерно так:

construstor TStorageHeader.InternalCreate(aPosition: Int64);
begin
 inherited Create;
 FillChar(f_Data, SizeOf(f_Data), 0); // - инициализируем данные
 f_Position := aPosition;
 LoadData(f_Position, f_Data, SizeOf(TData)); // - тут читаем данные 
end;

class function TStorageHeader.Create(aPosition: Int64): TStorageHeader;
var
 l_H :TStorageHeader; 
begin
 Assert(aPosition >= 0); // - все ПРЕДУСЛОВИЯ переносим из КОНСТРУКТОРА в ФАБРИЧНЫЙ МЕТОД
 // Далее - ВЕСЬ код ПОТЕНЦИАЛЬНО поднимающий исключения переносим из конструктора в ФАБРИЧНЫЙ МЕТОД:
 LockManager.LockFile(aPosition, SizeOf(TData)); // - тут блокируем регион файла и там может ПОДНИМАТЬСЯ ИСКЛЮЧЕНИЕ - LOCK_VIOLATION
 // - если исключение поднимется, то до конструктора (а стало быть деструктора) - дело не дойдёт
 //   ну и "флажков" заводить не надо
 l_H := InternalCreate(aPosition);
 Result := l_H;
end;

destructor TStorageHeader.Destroy;
begin
 SaveData(f_Position, f_Data, SizeOf(f_Data)); // - тут записываем данные 
 UnLockFile(f_Position, SizeOf(f_Data)); // - тут разблокируем регион файла
 inherited;
end;

Ну и ещё стоит прочитать вот что - BeforeRelease.

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

Ну и по-моему - "очевидно", что сохранять надо "неразрушенный объект".

И стоит (на мой вкус) вынести весь "сохраняющий код" из деструктора в BeforeRelease (BeforeDestruction).

https://groups.google.com/forum/#!topic/borland.public.delphi.language.objectpascal/d7ajc3wkngY

Цитирую:

"I just stumbled upon some behaviour of Delphi, which I find pretty 
confusing: I prefer using "AfterConstruction" and "BeforeDestruction" 
for creating/destroying class members. Now, if an exception is raised in 
"AfterConstrucion", Delphi immediately destroys the object (ok so far
but "BeforeDestruction" is not called (not ok). Online help is not clear 
about this, but one could guess that it is *always* called. My 
conclusion is: always override "Destroy" instead of "BeforeDestruction" 
to safely clean up. Is that right? What's the use of "BeforeDestruction" 
then?"

На самом деле - is ok.

Читаем документацию:

http://docwiki.embarcadero.com/VCL/2010/en/System.TObject.BeforeDestruction

Цитирую:

"BeforeDestruction is called automatically before the object's first destructor executes. Do not call it explicitly in your applications."

"Note:  BeforeDestruction is not called when the object is destroyed before it is fully constructed. That is, if the object's constructor raises an exception, the destructor is called to dispose of the object, but BeforeDestruction is not called."

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

Ну если у вас их нет, то тем более - рад за вас :-)

Не хочу никого делать "насильно счастливыми". Ну а вдруг...

Ну а может быть - "руки у меня кривые" и подобные проблемы только у меня, но "это вряд ли".

P.S.Ну и ещё (Коротко. "Почему нужны тесты") - ошибка "жила" уже лет 15-ть как. "Портила кровь и нервы", но "не мешала жить".

И при этом - сотни (а то и тысячи) пользователей (внутренних) работали с этим.

Но!

Стоило только сделать тесты (Коротко. Сделали нагрузочные тесты) - как ошибка - "сразу вылезла наружу". Хотя она была найдена не на "сотнях клиентов", а на двух.. Повторю на двух... Двух, но ДЕТЕРМЕНИРОВАННЫХ.

Вот и думайте... О "экстраполяции"...

На двух..

Скажу лишь - "Когда вы говорите "у меня нет на это времени", на самом деле вы имеете ввиду, что вам это не важно. Нет времени на саморазвитие? На спорт, на семью?".

Ну и ещё - "Только никто не контролирует разработчика, он сам себя контролирует – «бьёт по рукам». Но это не мазохизм. Взамен разработчик получает кое-что важное – ощущение стабильности и уверенности в своём профессиональном завтрашнем дне. А как творец – он творит в два раза больше. Именно творит, а не «пашет». Пашет он как раз меньше.".

9 комментариев:

  1. Я так подобное понимал сразу :-)
    Если в конструкторе может произойти исключение (а в случае работы с файлами запросто), то нужен, как вы называете, фабричный метод. Изолировать конструктор и подавать а выход годный объект. Я бы тут разделил конструктор и процедуру загрузки блока, а в класс-функции бы объединял их.

    Вообще конструкторов с потенциальными исключениями необходимо избегать. Это хорошее правило.

    Ну и код записи в деструкторе :-) ыыыы...
    Этого тоже следует избегать.
    И уж если невозможно избежать, то проверять, а надо ли записывать? По умолчанию объект инициализируется нулями, потому после загрузки нужно выставлять флаг отличный от нуля. Деструктор должен уничтожать, а не создавать и вычислять. Тоже хорошее правило, исключения из которого всегда заканчиваются отладчиком.

    Как-то так :-)

    ОтветитьУдалить
  2. Вот как сделан метод Free?
    if Self <> nil ten
    Destroy;

    В деструкторе что-то может делаться только с тем, что не равно нулю, False, Nil и т. д.

    ОтветитьУдалить
  3. Обычный баг кривой архитектуры. Использование конструктора и деструктора несколько не по назначению...

    ОтветитьУдалить
    Ответы
    1. "Обычный баг кривой архитектуры. Использование конструктора и деструктора несколько не по назначению..."

      У меня конечно "проблемы с артикуляцией" но пост был ИМЕННО про ЭТО.

      Удалить
    2. Формально говоря, сентенции относительно деструктора - это скорее недочёт, нежели ошибка.
      И устраняется этот недочёт очень легко, что указывает на то, что архитектура не настолько уж и "крива", как заявляет наш анонимный приятель...

      Удалить
    3. крива крива :-)

      я чуть позже напишу - "как прямее" :-)

      Удалить
    4. Но стоило "20 с лишним лет" проработать чтобы это начать понимать..

      не все же такие гении как "наш анонимный приятель" :-)

      Удалить
  4. Хотел отписаться здесь по теме ещё вчера, но вспомнил вдруг, что иногда "молчание - золото" ;-)
    Но тут смотрю - что-то "Анонимный" немного разошёлся IMHO... :-)
    "баг кривой архитектуры", "знал и раньше"... :-)
    Ну да Бог с Вами, уважаемый...

    По существу.
    Да, выполнять нагруженные действия в конструкторе и симметричные им в деструкторе - иногда соблазнительно.
    Когда у объекта модели есть состояние - логично для его представления в коде использовать класс, а протокол работы с ним свести к построению, какой-то обработке и освобождению, на этапах построения и освобождения выполняя симметричные действия, предваряющие и завершающие обработку.
    Например, в конструкторе выполняем блокировку и чтение, а в деструкторе — запись и разблокировку.
    К сожалению, по поддержка классов в Delphi не очень рассчитана на такое использование.
    В частности подразумевается, что финализация объекта должны быть всегда успешной, т.е. не приводить к необработанным исключениям.
    В приведённом примере TStorageHeader.Destroy к исключениям может привести как попытка сохранения, так и разблокировки участка файла и эти исключения не обрабатываются, что может быть источником загадочных утечек памяти в течение многих лет, поскольку вероятность таких событий относительно мала.
    Поэтому, переносить действия по обработке из этапа использования объекта на этапы его инициализации и финализации я бы не стал. Похожую тему ранее я затрагивал здесь.
    Я не знаю, что такое StorageHeader - могу только догадываться по названию, но этого недостаточно для того, чтобы однозначно судить о его назначении и о вариантах использования.
    Из общих соображений предложил бы в подобных случаях определить два метода: BeginUpdate и EndUpdate.
    BeginUpdate мог бы выполнять действия по блокировке фрагмента файла и чтению данных, а EndUpdate выполнял бы сохранение данных в случае, если в этом есть потребность (f_Data ведь изменяется между чтением в оригинале - в конструкторе и записью в деструкторе, иначе зачем перезаписывать блок данных в файле, если в нём не производилось изменений?) и разблокировал участок.
    Таким образом можно было бы разгрузить этапы конструирования и разрушения объекта от "нагруженных" действий, т.е. от операций, требующих синхронизации и накладывающих ограничения на обработку исключений.
    Ну и ещё. Потребовав по протоколу парности Begin/EndUpdate (вызову BeginUpdate необходимо должен соответствовать вызов EndUpdate) посредством простейшего счётчика можно было бы обеспечить кэширование изменений в f_Data перед внесением изменений в StorageHeader. Используя фабричный метод и интерфейсы (не совсем по назначению кстати, любезный "Анонимный" :-)) можно несколько упростить описанную схему - о близких к обсуждаемой теме вещах я говорил здесь.

    Извиняюсь перед автором поста, если понял его неправильно.

    ОтветитьУдалить
    Ответы
    1. "Ну да Бог с Вами, уважаемый..."

      -- как сказал один мой коллега - "ключевое слово "знал и раньше"" :-)

      Ну а вот - "я - не знал" :-) Не боги горшки обжигают...

      Спасибо за поддержку.

      Удалить