Небольшая, но анноящая утечка памяти, помогите исправить

← →
Дмитрий Белькевич © (2015-03-30 13:19) [0]

Что бы долго не мудрствовать, кину код:

TEPMessageQueue = class(TObject) private FList: TList; FLock: TCriticalSection;

procedure TEPMessageQueue.PushMessage(SID: cardinal; MessageLevel: TEPMessageLevel; MessageType: TEPMessageType; const AMessage: string; Urgent: boolean = False); var Msg: TEPMessage; begin FLock.Enter; try Msg := TEPMessage.Create; Msg.FSID := SID; Msg.FLevel := MessageLevel; Msg.FType := MessageType; Msg.FMessage := AMessage; if Urgent then FList.Insert(0, Msg) else FList.Add(Msg); finally FLock.Leave; end; end;

мэд и эврика показывает, что течет в "Msg := TEPMessage.Create;"

Как происходит. Проект работает, закрываю его "крестиком", как обычно, и иногда в указанном месте происходят утечки - штук по 3, по 5-7. Не то, что бы сильно нужно было, но люблю, что бы чистое везде всё было.

Как переписать код так, что бы Msg гарантированно добавлялся в TList?

← →
sniknik © (2015-03-30 13:55) [1]

а где у тебя освобождение занятого? раз утечка, значит не освобождается.

← →
sniknik © (2015-03-30 13:57) [2]

или просто вместо TList поставь TObjectList

← →
Дмитрий Белькевич © (2015-03-30 14:04) [3]

procedure TEPMessageQueue.Clear; var i: integer; begin FLock.Enter; try for i := 0 to FList.Count - 1 do begin TEPMessage(FList[i]).Free; FList[i] := nil; end; FList.Clear; finally FLock.Leave; end; end;

← →
DVM © (2015-03-30 14:05) [4]

> Как переписать код так, что бы Msg гарантированно добавлялся
> в TList?

Код деструктора TEPMessageQueue не приведен.
Если ты в деструкторе все же удаляешь объекты, то скорее всего твоя проблема в том, что в момент отработки деструктора в этот список продолжают падать сообщения. И получается, что после очистки списка в деструкторе в него опять накидывают сообщений.

← →
DVM © (2015-03-30 14:09) [5]

Поставь точку останова в Clear, а после попадания туда поставь точку останова в PushMessage, если попали туда, значит моя версия верна.

← →
Дмитрий Белькевич © (2015-03-30 14:35) [6]

destructor TEPMessageQueue.Destroy; begin Clear; FreeAndNil(FList); FreeAndNil(FLock); inherited; end;

PushMessage может, и, чаще всего, вызывается НЕ из основного потока программы. Сделана синхронизация с помощью FLock. Код, вообще, не мой, но не важно.

← →
junglecat © (2015-03-30 14:36) [7]

> [6] Дмитрий Белькевич © (30.03.15 14:35)

в PushMessage
try
if FList=nil then Exit;

← →
Дмитрий Белькевич © (2015-03-30 14:43) [8]

>if FList=nil then Exit;

FList там никогда не nil, иначе бы было AV, чего не наблюдается никогда.

← →
Дмитрий Белькевич © (2015-03-30 14:44) [9]

>FList там никогда не nil,

чатать: всегда не nil

← →
sniknik © (2015-03-30 14:44) [10]

я бы сделал так
procedure TEPMessageQueue.PushMessage(SID: cardinal; MessageLevel: TEPMessageLevel; MessageType: TEPMessageType; const AMessage: string; Urgent: boolean = False); var Msg: TEPMessage; begin if FLock=nil then Exit; FLock.Enter; try Msg := TEPMessage.Create; Msg.FSID := SID; Msg.FLevel := MessageLevel; Msg.FType := MessageType; Msg.FMessage := AMessage; if Urgent then FList.Insert(0, Msg) else FList.Add(Msg); finally FLock.Leave; end; end; destructor TEPMessageQueue.Destroy; begin FreeAndNil(FLock); Clear; FreeAndNil(FList); inherited; end;

а еще лучше
TEPMessageQueue = class(TObject) private FList: TObjectList; FLock: TCriticalSection; procedure TEPMessageQueue.PushMessage(SID: cardinal; MessageLevel: TEPMessageLevel; MessageType: TEPMessageType; const AMessage: string; Urgent: boolean = False); var Msg: TEPMessage; begin if FLock=nil then Exit; FLock.Enter; try Msg := TEPMessage.Create; Msg.FSID := SID; Msg.FLevel := MessageLevel; Msg.FType := MessageType; Msg.FMessage := AMessage; if Urgent then FList.Insert(0, Msg) else FList.Add(Msg); finally FLock.Leave; end; end; destructor TEPMessageQueue.Destroy; begin FreeAndNil(FLock); FreeAndNil(FList); inherited; end;

← →
junglecat © (2015-03-30 14:47) [11]

> [10] sniknik © (30.03.15 14:44)

Мы же в PushMessage ждем FLock, когда деструктор отработает, Clear освободит FLock, FList станет nil, в PushMessage мы снимемся с ручника, и достаточно проверить FList <> nil

← →
Дмитрий Белькевич © (2015-03-30 14:47) [12]

>а еще лучше

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

← →
Дмитрий Белькевич © (2015-03-30 14:50) [13]

>Мы же в PushMessage ждем FLock, когда деструктор отработает, Clear освободит FLock, FList станет nil, в PushMessage мы снимемся с ручника, и достаточно проверить FList <> nil

Да, так и есть. Странно - что AV не возникает. Ладно, перепишу, как sniknik кинул, посмотрю на результат, пока что всем огромное спасибо за участие!

← →
sniknik © (2015-03-30 14:58) [14]

> Мы же в PushMessage ждем FLock, когда деструктор отработает, Clear освободит FLock, FList станет nil, в PushMessage мы снимемся с ручника, и достаточно проверить FList <> nil
при входе
в деструкторе (клеар) "включили ручник", ждем-освобождаем, а PushMessage из потоков в это время происходит, и ждут на входе > FLock.Enter; в клеар освободили критическую секцию, и уже неважно, что там до входа, какая проверка, остальной код на добавление сработает.
поэтому и нужно отключат "рубильник" до того, первым, а не после.

ну, а FList: TObjectList;сам правильно Free вызовет.

← →
DVM © (2015-03-30 14:59) [15]

Все это костыли с проверкой на nil, тем более, что FreeAndNil не атомарна.

НЕНОРМАЛЬНО, кода объект уничтожается, но с ним пытаются продолжать работу. Проблема не столько в этом объекте, сколько во внешнем коде.

← →
DVM © (2015-03-30 15:07) [16]

По уму, надо стоппить те потоки, которые накидывают сообщения в очередь первыми, дожидаться, когда они закончат работу и ТОЛЬКО ПОТОМ убивать саму очередь.
В противном случае, как не ставь проверки, всегда будут коллизии. Их можно замаскировать многочисленными проверками, но это не устранит проблему плохого дизайна а отсрочит ее проявление.

← →
RWolf © (2015-03-30 15:12) [17]

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

← →
sniknik © (2015-03-30 15:20) [18]

> По уму, надо стоппить те потоки
в принципе да, но тут проверка тоже не помешает, только вместо выхода ексепт кидать.

← →
sniknik © (2015-03-30 15:26) [19]

+
можно кстати освобождение общего объекта сделать как reliase у форм, free делается в общем событийном обработчике... но тогда нужно все в этот обработчик переносить. и запись событием делать, и критическая секция тогда будет не нужна.

← →
Дмитрий Белькевич © (2015-03-30 15:43) [20]

Не всё так просто с TObjectList. Логично, что в комплекте пушу есть поп, имеем там такой код:

function TEPMessageQueue.PopMessage: TEPMessage; begin Result := nil; if FLock = nil then Exit; FLock.Enter; try if FList.Count > 0 then begin Result := TEPMessage(FList[0]); FList.Delete(0); end; finally FLock.Leave; end; end;

Понятно, что при FList.Delete(0) происходит - мы успешно прибиваем "бережно сохраненный" объект.

Согласен, что архитектура плохая, но такая уж есть, переписывать сложно - много всего завязано.

По поводу неатомарность FreeAndNil - то ссылка становится nil"ом раньше, чем освобождается объект, мне кажется, проблем быть не должно.

← →
Дмитрий Белькевич © (2015-03-30 15:48) [21]

В конце обработки извлеченной мессаги:

FreeAndNil(Msg);

← →
sniknik © (2015-03-30 15:51) [22]

> проблем быть не должно.
а вызов процедуры, передача параметра... на все время тратится, и в это время возможен вызов PushMessage из другого потока. даже на присвоении интеджера... что и означает сто операция не атомарна (эти не прерываются, некуда вклинить не желательные вызовы).

т.е. проблемы будут, редко но будут.

← →
Дмитрий Белькевич © (2015-03-30 15:56) [23]

посмотрел код еще раз - TEPMessage как класс не нужен :) в смысле - зачем его вообще сделали классом - не ясно, там записи вполне хватает, TEPMessage вообще разрушать и создавать не нужно. что тут скажешь :) буду переписывать.

← →
RWolf © (2015-03-30 16:02) [24]

Как это — создавать не нужно? а в контейнер что помещать, локальную переменную со стека, что ли?

← →
DVM © (2015-03-30 16:03) [25]

> Дмитрий Белькевич © (30.03.15 15:56) [23]

> там записи вполне хватает, TEPMessage вообще разрушать и
> создавать не нужно.

Не торопись.
New() и Dispose() там тоже не будет?

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

New + Dispose + TThreadList попробую, у меня похожее решение в другой части кода используется, там дизайн, правда, лучше.

procedure TEPMessageQueue.PushMessage(SID: cardinal; MessageLevel: TEPMessageLevel; MessageType: TEPMessageType; const AMessage: string; Urgent: boolean = False); var Msg: TEPMessage; begin FLock.Enter; try Msg := TEPMessage.Create; try Msg.FSID := SID; Msg.FLevel := MessageLevel; Msg.FType := MessageType; Msg.FMessage := AMessage; if Urgent then FList.Insert(0, Msg) else FList.Add(Msg); except Msg.Free; raise; end; finally FLock.Leave; end; end;

сообщений бывает много, насчет пула TEPMessage - хорошая мысль, спасибо, подумаю.

Т.е логика такая: в аллокаторе есть 2 потокозащищенных списка:
1) FreeObjects (очередь)
2) UsedObjects
При старте программы аллокатор создает, скажем 1000 обхектов TEPMessage и кладет их в список 1).
Тот, кому нужен экземпляр TEPMessage, не вызывает конструктор, а вызывает метод аллокатора, скажем GetTEPMessage и получает свой экземпляр. Одновременно этот же экземпляр кладется в список 2), а из первого удаляется.
Если в списке 1) нет больше свободных объектов, то создается новый, который туда помещается. Далее- аналогично.
Удалять объекты должен будет аллокатор по обоим спискам в самом конце работы программы.
Аллокатор можно сделать синглтоном.

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

У меня вопрос: объект типа TEPMessageQueue - он глобальный на всё время работы приложения, или периодически создаётся/уничтожается в процессе работы?

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

> KSergey © (30.03.15 16:36) [32]

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

Лучше бы, конечно, почистить их, да.

Часть процесса отладки, является логирование. Пиши в лог создание класса а стэк вызова, а на разрушении пиши тот-же стек, ты даже не поверишь, как все упростится :)

Удалось всё таки закрыть очередь сообщений от попадания в неё строк после разрушения, добавил несколько флагов вне очереди. Переписал с объектов на записи (New + Dispose + TThreadList). Пока утечек не наблюдаю. Думаю, скорость тоже может несколько вырасти. В общем - всем спасибо!

Небольшая, но анноящая утечка памяти, помогите исправить Найти похожие ветки