What do you think, Stefan Glienke?
What do you think, Stefan Glienke?
#Spring4D
http://stackoverflow.com/questions/38162508/why-is-list-of-type-tobjectlist-freed-automatically-after-iteration
#Spring4D
http://stackoverflow.com/questions/38162508/why-is-list-of-type-tobjectlist-freed-automatically-after-iteration
Never store a reference of TObjectList in a variable of the classtype.
ReplyDeleteAlso, instead of TObjectList.Create, you should use TCollections.CreateObjectList.
This way, you only need Spring.Collections in your uses.
"Never" mix classes and Interfaces.
ReplyDeleteMixing class instance and interface references has always been asking for trouble, by design. A warning about this might be a useful addition to FixInsight (if it's not already included).
ReplyDeleteThe sample code does not "mix" classes and interfaces - it just uses class variables, but the underlying framework is designed to use interface variables instead, hence the problem.
ReplyDeleteBTW Nowadays this basic rule has to be "Never mix class and interface references when you are not with ARC".
ReplyDeleteBut that is also not the real truth. What about TInterfacedPersistent?
Never mix class and interface references if the implementing class has reference counting and the ARC is not with you!
With ARC the rule is: "No rule, do it"
Sergey Kasandrov It does: it keeps a local variable referencing TObjectList (instance) and then it uses the for ... in enumerator (interface). I agree with Rudy's comment that interface-only usage should be enforced by hiding the classes in the implementation, exposing only interfaces. Especially when the interface usage is non-obvious like here. Either that, or the enumerator should be a separate reference-counted class, created/destroyed as needed for each for .. in scope. Or, if the enumerator is implemented by the same class like here then it should not be reference counted at all (I mean, it should reimplement _AddRef and _Release to do nothing).
ReplyDeleteOndrej Kelle instance reference refers TObjectList, interface reference refers its enumerator, these are different things. I don't know the framework, I just don't understand why the enumerator's ctor and dtor include _Addref/_Release calls. IMO they just should be removed, and that solves the whole problem.
ReplyDeleteSergey Kasandrov Isn't the enumerator implemented by the TObjectList itself? I have to check the code again...
ReplyDeleteOndrej Kelle How will that "hiding" help in case of
ReplyDeletevar
foo: IFoo;
begin
foo := GetFooFromWhereEver();
TObject(foo).Free;
end;
And I do not accept "Don't do this" because you also do not accept it in case of
var
list: TList; // dont do this, use IList instead
begin
list := TList.Create();
end;
I dislike hiding the implementing class in the implementation section, it can be handy for testing and workarounds to have access to it.
ReplyDeleteInstead I keep the interface declaration and factory functions in the "naturally" named unit, for example "MyCollectionLib.Lists", and the implementing class in a "private" unit, like "MyCollectionLib.Lists.Detail". The user should never be required to use the private units.
Ondrej Kelle I don't know how Spring4D implement the enumerator, I use dedicated refcounted classes for the enumerators.
ReplyDeleteSergey Kasandrov IMHO that makes sense and would solve the problem. But I still have to check the code again.
ReplyDeleteOliver Münzberg Regarding TObject(foo).Free; Everybody is free to shoot themselves in the foot. ;-)
ReplyDeleteOndrej Kelle Well, you describe the use case of the spring collections.
ReplyDeleteSergey Kasandrov Ondrej Kelle No, it will just produce trouble when you use spring Enumerable
ReplyDeleteHence I made the point of just using TCollections.CreateXYZ instead of TXYZ.Create.
ReplyDeleteThe functions of TCollections only return interface references and - more importantly - allow for code folding[1] starting with Spring4D 1.2.
If you want the concrete classes to be hidden, file an improvement or make a pull request with the changes at Spring4D.org.
[1] Only for Lists: reducing binary size by using a specialized TObjectList instead of TObjectList for every T but, still returning IList.
The main problem with delphi is, that you had to know some implementation details about the reference handling. A class may implement an interface and that interface implementation may have reference counting or not. You will never know without looking at the documentation or the source.
ReplyDeleteThere is no general rule, except that there is no general rule and read the docs and the source.
The enumerator is a separate class (TList.TEnumerator). It holds a reference to the list (TList) instance and also calls _AddRef/_Release in its constructor/destructor. That means Spring4D internally mixes instance/interface references itself already. From the "Getting Started" demo, I guess Spring4D is simply meant to be used with interfaces only by the calling code.
ReplyDeleteWhen I comment out those _AddRef/_Release calls in TList.TEnumerator constructor/destructor, nothing bad happens ;-) and the problem seems to be solved: for..in doesn't destroy the list when it gets out of scope. In other words, Sergey Kasandrov your suggestion seems to work. The question is still, is it dangerous/can it cause a memory leak? At the moment I can't imagine a case when it would...
ReplyDeleteOh yeah and I forgot to mention I also always name my interface-implementing classes with an "Impl" suffix, so in this case I would have an "IObjectList" interface in "MyCollectionLib.Lists.pas" and a "TObjectListImpl" class in "MyCollectionLib.Lists.Detail.pas".
ReplyDeleteAsbjørn Heid That would be the "clean" approach, however I suspect Stefan Glienke and contributors also had to fight with internal compiler errors and similar nasties.
ReplyDeleteOndrej Kelle Yeah, not unlikely given the sad state of generics not long ago.
ReplyDeleteWasn't meant as a criticism by the way, just sharing my experience.
Ondrej Kelle I looked at my code, and it turned out that I used _Addref/_Release in the same place too; it is logical but on the second thought it is probably unnecessary (my enumerator is kinda lower-level, _Addref/_Release does not lead to any problem because ... I don't use classes at all :) - https://bitbucket.org/sergworks/tforge/src/1ebd4d756894e26e5153761586833afdaad9e59d/Source/Engine/Forge/tfByteVectors.pas?at=default&fileviewer=file-view-default
ReplyDeleteThe _AddRef/_Release calls are neccessary in that case
ReplyDeletevar
list: IList;
enumrate: IEnumerator;
begin
list := TObjectList.Create(true);
list.Add( TObject.Create );
enumerate := list.GetEnumerator();
list := nil;
// the next line will raise an exception
// if _AddRef/_Release are not called from TEnumerator
while enumerate.MoveNext() do
begin
end;
end;
Oliver Münzberg this is obviously bad code, it does not work anyway, with or without _Addref/_Release.
ReplyDeleteSergey Kasandrov I had no problem with this code (without the suggested fix). And why is it bad? Are we again by some "don't do this"?
ReplyDeleteSo there are some "don't do this" you accept and others you don't. I don't get your point how you decide this? Current temperature of the water?
Oliver Münzberg I don't use Delphi classes at all and have no problems with reference counting like the one discussed here. I have no "don't do this" because of refcounting design..
ReplyDeleteSergey Kasandrov Sorry, I had misunderstand "obviously bad code, it does not work" to be a "don't do this"
ReplyDeleteOndrej Kelle These will break all cases where you have a Interface to the Enumerator but not longer to the List itself. There a lot of cases where you can work only with the enumerators and using the list only for pick up the initial Data. This is one of the cool things in Spring4d. If we use these great library we have to follow the rules as always. And these fix will only help if there is a mix between class and interface use :-(
ReplyDeleteFriedrich Westermann
ReplyDeleteI haven't thought about this scenario. Thanks for pointing it out.
Friedrich Westermann +Oliver Münzberg - If so then the right design would be to have 2 separate enumerators - one to support for .. in iteration and another for other purposes. The other question would be is it worthwhile to write that, since it is needed only for "mixing object and interface references".
ReplyDeleteSergey Kasandrov I don't think so. Mixing is always bad and we should always avoid it.
ReplyDeleteSergey Kasandrov The right design will bring you just some simple "don't do this in that way" rules.
ReplyDeleteWorking with delphi we had those rules we all should know:
- Do not free a property
- Use TXMLDocument with an owner or use it as IXMLDocument
- Do not cast an interface reference to TObject and free it when the implementation has refcounting
- Do not free an instance when the lifetime is delegated to ... (f.i. a TObjectList)
- Do not produce dangling pointer
- ...
So it is common to have such rules. Reducing that rules will lead to code that is nearly impossible to test or classes you cannot inherit from. Inheritance is the basic idea of OOP and testing is the base for building code you can rely on.
On my own code, I enforce the "use the interface not the class" by having all the methods/properties in the class at least protected. That way you can inherit if needed, and then use/test through the interface. But having the class reference will not expose anything useful. However I'm not sure if this is affordable in Spring4D
ReplyDeleteI guess this is more of a documentation problem rather than a bug in the source (or design). It is said that the Spring4D collections are meant to be used as interfaces and indeed are designed as such (since many things need the refcounting internally). If you have an enumerator it is as designed that it keeps alive its source because they are used for more than just for-in loops. For example in the "extension methods" like Where and so on that return IEnumerable because they as well keep their source alive via interface reference counting.
ReplyDeleteI know people are arguing that if you just use the list as object and don't mix it yourself it should be working and I might consider doing the _AddRef and _Release only if the referenced list already has a RefCount > 0 so using the list as object and using it in a for-in loop will work but it will just shift the problem somewhere else (anyway I might consider doing this change).
Hiding the classes in an implementation part is not possible because then this would break any possibility to inherit from these classes and extend them (as I know people do). Also I will not jump through some extra hoops just to make it impossible to use them as classes in your code but rather put documentation on them telling that you should use TCollections.Create*** and their appropriate interfaces instead.
Stefan Glienke There would be a possibility to avoid this behaviour:
ReplyDeleteThe items of the list could be organized by an interfaced container and the enumerator will increase the refcount on the internal items container and not on the list itself. So the items will get freed when no list or enumerator holds that items container.
But the great question: What are we going to solve with this?
Oliver Münzberg Are you suggesting to replace a simple dynamic array of T as backing storage with an array of containers for T just to solve a problem that is not really a problem? No way this is gonna happen :)
ReplyDeleteStefan Glienke No, I suggest to replace the whole array with an interfaced container (maybe IArray).
ReplyDeleteBut I also mentioned: "What are we going to solve with this?"
Yet another interface would place an unacceptable overhead on the list class. TList already basically is the implementation behind that IArray interface.
ReplyDelete"What are we going to solve with this?"
Making one of many possible unintended usages of the library possible while the next one is waiting to blow up (like using Where on a TList reference).
Sure I could go and make all of these methods protected so people cannot use them but then I ask: why use Spring4D collections at all and not just use those from Generics.Collections? Because Spring4D collections have no benefit at all over those when not used as interfaces.
Stefan Glienke That's another reason why I like placing the implementations in their own, specially named units. Then the users can derive and extend, but it is explicit that they then rely on implementation details of the library. Usually fine, but potentially more brittle during updates.
ReplyDeleteIt's really amusing to watch the endless discussion on SO. In the end it's just one basic rule:
ReplyDeleteNever use object references to classes that inherit from TInterfacedObject because the reference counting can kick in and destroy your instance anywhere (or manually call _AddRef/_Release).
Another thing some people seem to misunderstand is that GetEnumerator is not just there to be called by a for-in loop. Any code can do this and keep the returned enumerator around for as long as it wants. And when using reference counting you can very nicely ensure that the original source also stays alive as long as it needs to.
That simple rule seems to be too much for some to grasp .....
ReplyDeleteStefan Glienke Why is it impossible to keep GetEnumerator private or protected and never call it, and have different enumerator for direct call, with _Addref in constructor and _Release in destructor, derived from the base enumerator class by just overriding the constructor and destructor?
ReplyDeleteSergey Kasandrov Because I am not spending any time on making the collections to be used as object references. They are to be used via interfaces, period.
ReplyDeleteThis is just one problem surfacing when using them as object references. Yes, I could make this specific use case work. But what comes next? Someone passing his TObjectList to some method that asks for IList because that works and then wondering why the list got destroyed after? Someone iterating over list.Where(...) and then wondering why the list is gone? Yes, I can then make the Where and all other methods on the class protected so nobody ever calls them when using the lists as objects.
But all this would be pointless because if you want to have classes, then use System.Generics.Collections.
Stefan Glienke These was my point on SO, but some people jumps in without using or know about Spring4D. I love these library. With the simple nomix rule it is asuperb library. thanks for your work
ReplyDelete