Comments 32
В задаче 2, извините, говнокод. Понятно, что после Free ссылка на obj не обнулится, но никто не гарантирует, что внутри Remove не будет попытки разыменования obj.
+1
Даже если внутри Remove не обращаются к состоянию Obj, а используют только указатель в качестве ключа, то в многопоточном приложении это может привести к проблемам. После вызова Obj.Free в коллекцию из другого потока могут успеть положить новый объект по этому же адресу.
+1
TDictionary не thread safe. Удаление без блокировок не корректно в принципе. Поэтому давайте для простоты положим, что код однопоточный.
0
А код в секции //DoSomething может влиять? Я уже больше 10 лет не брал в руки шашки Delphi, нужно по хорошему в дизассемблере смотреть что там происходит. Возможно оптимизации влияют на поведение.
По первой задаче, все может развалится если в параметр AExtraData засунуть ссылку на элемент из массива при условии что на него больше никто не ссылается. Например вызов AddDataToAll(DataArr[0]) уничтожит объект в DataArr[0] на первой итерации, а на второй мы можем развалиться (use after free).
По первой задаче, все может развалится если в параметр AExtraData засунуть ссылку на элемент из массива при условии что на него больше никто не ссылается. Например вызов AddDataToAll(DataArr[0]) уничтожит объект в DataArr[0] на первой итерации, а на второй мы можем развалиться (use after free).
0
Да, я думаю, что вы близки к истине, учитывая, что автор пишет, что данный код иногда падает. А «иногда» — очень хороший признак многопоточности.
+1
obj сначала нужно удалить из коллекции, а потом делать obj.Free, не?
0
первый «пример» — не понятно чего автор ожидал от кода, при условии, что интерфейсы относятся к сущностям с автоматическим управлением памятью, т.е. как только ссылок на элемент не останется, он будет уничтожен
0
Представьте например, что Data1 и Data2 — хранят числа, а CreateMerged_IData создает новый IData с суммой этих чисел.
Вызывая procedure AddDataToAll(const AExtraData: IData); я ожидаю, что число, хранящееся в AExtraData добавится ко всем числам, хранящимся в DataArr (ну и в DataArr само собой будут новые интерфейсы, которые хранят новую сумму).
Вызывая procedure AddDataToAll(const AExtraData: IData); я ожидаю, что число, хранящееся в AExtraData добавится ко всем числам, хранящимся в DataArr (ну и в DataArr само собой будут новые интерфейсы, которые хранят новую сумму).
0
думаю, что const нужно прописать у параметров функции, чтоб подсчёт ссылок не путался под ногами при вызове, тем более по коду один и тот же элемент массива сначала передаётся в функцию и внутри неё заменяется.
А вообще стиль кодирования в этих примерах какой-то «опасный», «неряшливый». То интерфейсы в функцию без const, то объект освобождаем, а потом используем. Страшно…
А вообще стиль кодирования в этих примерах какой-то «опасный», «неряшливый». То интерфейсы в функцию без const, то объект освобождаем, а потом используем. Страшно…
0
В первом — не используется переменная цикла:
AddData(DataArr[0], AExtraData, DataArr[0]); => AddData(DataArr[i], AExtraData, DataArr[i]);
Во втором — не верный порядок удаления элемента.
obj.Free;
FCollection.Remove(obj);
obj := nil;
=>
FCollection.Remove(obj);
obj.Free;
obj := nil;
Вроде все. Задачки для пятиклассника, если честно.
AddData(DataArr[0], AExtraData, DataArr[0]); => AddData(DataArr[i], AExtraData, DataArr[i]);
Во втором — не верный порядок удаления элемента.
obj.Free;
FCollection.Remove(obj);
obj := nil;
=>
FCollection.Remove(obj);
obj.Free;
obj := nil;
Вроде все. Задачки для пятиклассника, если честно.
0
AddData(DataArr[0], AExtraData, DataArr[0]); => AddData(DataArr[i], AExtraData, DataArr[i]);Миль пардон. Это уже моя ошибка когда набирал пример. Большое спасибо за замечание, поправил. :) Подводный камень там в другом.
Во втором — не верный порядок удаления элемента.Ну если допустить, что так (хеш берется от указателя), то объяснить почему можете?
0
Ну если допустить, что так (хеш берется от указателя), то объяснить почему можете?
habrahabr.ru/post/269359/#comment_8624505
0
>Подводный камень там в другом.
В потере указателей на объекты? Создаем новый объект и подменяем ими старый. Сборщика мусора нет. Других проблем вроде не вижу (((
>Ну если допустить, что так (хеш берется от указателя), то объяснить почему можете?
Честно — нет, я уже подзабыл delphi капитальненько… Но, даже по простой логике — сначала удаляем из коллекции, потом из памяти). Как уже сказал Zapped, использовали-бы FreeAndNil — не было-бы подобного вопроса)
В потере указателей на объекты? Создаем новый объект и подменяем ими старый. Сборщика мусора нет. Других проблем вроде не вижу (((
>Ну если допустить, что так (хеш берется от указателя), то объяснить почему можете?
Честно — нет, я уже подзабыл delphi капитальненько… Но, даже по простой логике — сначала удаляем из коллекции, потом из памяти). Как уже сказал Zapped, использовали-бы FreeAndNil — не было-бы подобного вопроса)
0
«obj.Free; obj := nil» => FreeAndNil (obj)
была бы привычка у автора использовать FreeAndNil, не было бы «загадки» 2
была бы привычка у автора использовать FreeAndNil, не было бы «загадки» 2
0
во поводу 1:
лучше использовать функцию, вместо «процедура + out parameter»
а если пример не упрощён, то ошибка в том, что DataArr не инициализирован (нет SetLength(DataArr, ...)), а все эти интерфейсы — так, отвлекающий манёвр
лучше использовать функцию, вместо «процедура + out parameter»
function AddData(Data1, Data2: IData): IData;
begin
Result := CreateMerged_IData(Data1.Ptr, Data2.Ptr); //функция Create_IData создает новый экземпляр IData
end;
var DataArr: array of IData;
procedure AddDataToAll(const AExtraData: IData);
var i: Integer;
begin
if not Assigned(AExtraData) then
Exit;
for i := 0 to Length(DataArr) - 1 do
DataArr[i] := AddData(DataArr[i], AExtraData);
end;
а если пример не упрощён, то ошибка в том, что DataArr не инициализирован (нет SetLength(DataArr, ...)), а все эти интерфейсы — так, отвлекающий манёвр
0
Если в первом примере в AddDataToAll
строку
AddData(DataArr[i], AExtraData, DataArr[i]);
заменить на
var i: Integer; temp: IData;
temp := DataArr[i];
AddData(temp, AExtraData, DataArr[i]);
то всё работает.
Причина, видимо, в том, что при вызове процедуры AddData out параметр инициализируется nil.
а так как он ссылается на «живой» параметр, то, соответственно, в массиве по этому индексу окажется nil.
но такое поведение out параметров описано в справке Delphi.
Я лично на Дельфи не пишу, но те, кто использует Дельфи должны о таком поведении знать.
строку
AddData(DataArr[i], AExtraData, DataArr[i]);
заменить на
var i: Integer; temp: IData;
temp := DataArr[i];
AddData(temp, AExtraData, DataArr[i]);
то всё работает.
Причина, видимо, в том, что при вызове процедуры AddData out параметр инициализируется nil.
а так как он ссылается на «живой» параметр, то, соответственно, в массиве по этому индексу окажется nil.
но такое поведение out параметров описано в справке Delphi.
Я лично на Дельфи не пишу, но те, кто использует Дельфи должны о таком поведении знать.
+1
кхм, я не спец по дельфям, но набросал у себя в блокноте следующее (пусть будет и здесь, чтобы потом сравнить с правильными ответами):
Задача 1:
не знаю, но подозреваю:
уж не будет ли выполнен OutData.Release() в функции AddData() до вызова функции CreateMerged_IData() ???
(что-то мне звучит бредово, но других мыслей нет, да и когда в дельфях не было досадных багов и/или недоработок? по-хорошему, надо в дизассемблере это предположение проверять)
Задача 2:
После выполнения obj.Free() переменная obj сохраняет значение (obj <> nil), и в старых дельфях код должен работать как часы…
Но как быть с современными версиями дельфей, которые умеют «автоматический подсчет ссылок» ???
( инструкция «obj := nil» по сути есть «пинок» нынешним дельфям для выполнения «obj.Release()», но память obj^ к этому моменту может быть уже невалидной! разве нет? )
P.S.:
отчаянно не хватает информации о версии Delphi, на которой код работает неправильно ))
Задача 1:
не знаю, но подозреваю:
уж не будет ли выполнен OutData.Release() в функции AddData() до вызова функции CreateMerged_IData() ???
(что-то мне звучит бредово, но других мыслей нет, да и когда в дельфях не было досадных багов и/или недоработок? по-хорошему, надо в дизассемблере это предположение проверять)
Задача 2:
После выполнения obj.Free() переменная obj сохраняет значение (obj <> nil), и в старых дельфях код должен работать как часы…
Но как быть с современными версиями дельфей, которые умеют «автоматический подсчет ссылок» ???
( инструкция «obj := nil» по сути есть «пинок» нынешним дельфям для выполнения «obj.Release()», но память obj^ к этому моменту может быть уже невалидной! разве нет? )
P.S.:
отчаянно не хватает информации о версии Delphi, на которой код работает неправильно ))
0
отчаянно не хватает информации о версии Delphi, на которой код работает неправильно ))
Это не важно. Это не баг, а подводный камень. Код работает правильно, но не очевидно на первый взгляд. Так что будет проявляться во всех версиях делфи.
0
ну, я о том, что старые версии дельфей не умели делать подсчет ссылок для объектов, (и ссылки подсчитывались только для интерфейсных переменных), а нынешние — умеют, хотя вроде есть директива, это поведение отключающая
0
а нынешние — умеют, хотя вроде есть директива, это поведение отключающая
Нет. Нынешние не умеют считать ссылки для объектов. Иначе бы это сильно поломало обратную совместимость.
0
ой, ну да, вторая задача по-любому только на современных версиях дельфей работать будет, там же генерик используется ))
что-то сразу и не обратил внимания на генерик )
что-то сразу и не обратил внимания на генерик )
0
2) С учётом, что TObject.Free ничего страшного не делает, остается вариант, что в FCollection.Remove(obj) приходит нулевой указатель, а из дельфийского папира вроде как выходит, что значение nil недопустимо
0
Так как у камрадов появился вопрос: «как в Remove может попасть nil, если перед этим стоит Obj.Free, оно же должно крешится на этом».
Это не так:
var Obj: TObject;
…
Obj := nil;
Obj.Free;
не вызовет никаких «крешей». Возможно, что именно эта строчка — NIL.Free рассматривается как неочевидное и невероятное.
Это не так:
var Obj: TObject;
…
Obj := nil;
Obj.Free;
не вызовет никаких «крешей». Возможно, что именно эта строчка — NIL.Free рассматривается как неочевидное и невероятное.
0
Опубликовал ответы.
0
спасибо!
очень интересные ответы. особенно по первой задаче )
я некоторых тонкостей не знал в плане передачи интерфейсных параметров, а про «out-» и вовсе не подозревал, как именно там устроено.
буду знать… и для надежности сохранил страничку на жесткий диск )
очень интересные ответы. особенно по первой задаче )
я некоторых тонкостей не знал в плане передачи интерфейсных параметров, а про «out-» и вовсе не подозревал, как именно там устроено.
буду знать… и для надежности сохранил страничку на жесткий диск )
0
Пожалуйста. Жаль только, что в целом статью отхабрили.
0
в смысле? это значит, что плюсов нету?
ну, сорри, я бы плюсанул, но у меня «недостаточно кармы для голосования» ((
ну, сорри, я бы плюсанул, но у меня «недостаточно кармы для голосования» ((
0
В текущий момент минусов больше чем плюсов. Я за плюсами не гоняюсь, просто когда статья набирает хоть что-то — видно что народу интересно, а сейчас я не вижу смысла писать еще подобные статьи про «подводные камни».
p.s. А вообще давайте заканчивать говорить про плюсы/минусы. Тут такое не в почете, да и сам я это не люблю. Просто сделал для себя выводы касательно этой тематики статей.
p.s. А вообще давайте заканчивать говорить про плюсы/минусы. Тут такое не в почете, да и сам я это не люблю. Просто сделал для себя выводы касательно этой тематики статей.
0
Sign up to leave a comment.
Что не так с этим кодом?