Mantis - Squeak
Viewing Issue Advanced Details
6347 Kernel minor always 03-18-07 13:42 09-25-08 23:46
0006347: [FIX] testFinalizationOfEquals fails
ObjectFinalizerTests>>fails, because not all string objects created get finalized. This is no surprise: the method itself holds on to the last object that supposedly ought to get finalized. The attached patch fixes this.
 oftfix.1.cs [^] (554 bytes) 03-18-07 13:42
 FinalizationRegistry_KeyIdentity_Test_M6347-nice.1.cs [^] (1,533 bytes) 09-23-08 20:44
 FinalizationRegistry_KeyIdentity_Patch_M6347-nice.1.cs [^] (1,328 bytes) 09-24-08 19:48
 FinalizationRegistry_KeyIdentity_Test_M6347-nice.2.cs [^] (1,411 bytes) 09-25-08 23:43

04-15-07 10:04   
ObjectFinalizerTests don't fail in 3.10.
Some is wrong ?
04-15-07 17:54   
The method #testFinalizationOfEquals has been removed in 3.10 (7081); it is still present in the 3.9 image. So, certainly the test passes, as the failing test has been deleted.
04-15-07 20:20   
Well , yours is not a fix, is the old testFinalizationOfEquals in 3.9.
Ask Ralph for what to do

06-18-08 11:18   
This is not a fix. The problem below is that Object>>finalizationRegistry is a WeakRegistry. WeakRegistry uses a WeakKeyDictionary to hold "objects to finalize".
If you add an object that is equal to an object in the registry the first one gets replaced by the second one.

You fix only works if there is GC run between the two loops.

A fix would be to change WeakRegistry valueDictionary to a WeakIdentityKeyDictionary. Other classes using WeakRegistry are OldSocket, Socket, StandardFileStream.
nicolas cellier   
09-23-08 20:04   
I can understand this code was not understood: without your usefull comments, its goals are quite obscur to me. Maybe a better test would be

| finalizationRecorder object |
finalizationRecorder := Set new.
object := 'hello'.
object copy toFinalizeSend: #add: to: finalizationRecorder with: 1.
object copy toFinalizeSend: #add: to: finalizationRecorder with: 2.
Smalltalk garbageCollect.
self assert: finalizationRecorder = #(1 2) asSet description: 'each object copy should have been finalized.'

Did you ever notice how children learn addition easily and subtraction more painfully? Same must be true for add:/remove: in our poor programmer minds!

nicolas cellier   
09-23-08 20:40   
Not sure that above test fails 100%. Because the second copy can trigger a scavenge and finalize first copy before second is added to the registry.
One more obscure way to write it is:

    ((object copy)
        toFinalizeSend: #add: to: finalizationRecorder with: 1;
        copy) toFinalizeSend: #add: to: finalizationRecorder with: 2.

Another option would be:

#(1 2) inject: object into: [:anObject :aNumber |
    (anObject copy)
        toFinalizeSend: #add: to: finalizationRecorder with: aNumber;
(#yourself would fail because block arg :anObject would reference last copy).

Well, still too cryptic to my taste.
I have opted for an overly verbose test implementation in FinalizationRegistry_KeyIdentity_Test_M6347-nice

Feel free to shrink it.

09-24-08 08:40   
I don't understand what you are after. The test is called finalization of equals. How about writing it this way

| finalizationRecorder object o1 o2 |
finalizationRecorder := Set new.
object := 'hello'.
(o1 := object copy) toFinalizeSend: #add: to: finalizationRecorder with: 1.
(o2 := object copy) toFinalizeSend: #add: to: finalizationRecorder with: 2.
self assert: finalizationRecorder size = 0.
o1 := o2 := nil.
Smalltalk garbageCollect.
self assert: finalizationRecorder size = 2.
nicolas cellier   
09-24-08 16:37   
hi noha (norbert?),
I was just feeling that getting rid of o1 and o2 references could make code even more clear, but did not succeed, forget it.
The code you proposed is exactly FinalizationRegistry_KeyIdentity_Test_M6347-nice.1.cs, though i made it much more verbose (probably too much).

I like very much this one:
self assert: finalizationRecorder size = 0.

To me, both verbose ("mine") and lean version ("yours") are acceptable.
Maybe I prefer the lean one.
But we both have biased opinion about judging intention revealing...
...we know the intention before reading the code.
Only a third could tell if he better understand the verbose or the lean version.
(and he should not read above comments!).

09-24-08 16:57   
Hi Nicolas,

it's Norbert actually :) And you are right, the changes are equal. It's embarassing for me as it uncovered the fact that I didn't take a look at yours :)) Sorry.

I think you don't need

self assert: copy1 = copy2

You are testing if copy is working as intended and that is not the right place. :)

Are you sure that

      assert: finalizationRecorder = #(1 2) asSet
      description: 'each object copy should have been finalized.'.

always works. I'm not sure as finalizationRegistry is a Dictionary if the order of executions is the same as the order of addition. That's why I chose "size" selector instead. Lazy me!

You spent really much time into single small issues. That's fanatastic to have such a motivation. Bravo! Makes sense completely.
nicolas cellier   
09-24-08 21:16   
SUnit TestCase are specifications.
As such, it is most important that their purpose be very clear.
    "Ce qui se conçoit bien s'énonce clairement."
Not sure I understand every Squeak TestCase by just reading it.
Ideally, I should.
TestCase can also serve as coding tutorials, and that's how I tried to exercize.
To get a newcomer understanding the code while not boring an advanced user is a challenge.
These considerations make this example a very interesting TestCase to study.

    self assert: copy1 = copy2.
is not a goal per se, it is a pre-requisite to trigger the bug/test the feature.
it would far better be expressed:
    self prerequisite: copy1 = copy2.
For example, If we would refactor using
    object := Object new.
then the bug would not show up. That's why i thought it was important.
Of course, there is some redundancy with method selector name and comments...
It can as well be removed.

You could as well ask why stopping half way and not protecting against:
    object := $o.
with a:
    self prerequisite: copy1 ~~ copy2.
though "mathematically" as necessary as the above?
Well, I just don't know where to put the limit...
That's why I started a discussion.

Hope this example can seed some more expressive coding,
just to compensate for my currently dropping productivity!

Ah, and Set are unordered too.
09-24-08 22:19   
Ah, the goal is to be clear? :) Ok!

I don't think the assert is a stronger argument than putting a comment in front of it. For me it is kind of "separation of concern". For me

copy1 := anObject copy.
copy2 := anObject copy.
self assert: copy1 = copy2

is a test if the copy functionality is working correct. IMHO it is not the right place to put it. You should be able to assume that copy1 = copy2 is true as you constructed the objects. To create an object just to make two copies is too complicated. Using just strings is ok for the purpose of test

"create two arbitrary objects that are distinct but equal"
object1 := 'some string' copy.
object2 := 'some string' copy.

shows it very clear.

A different :) implementation could be:

| keeper receiver |
receiver := OrderedCollection new.
"add two equal objects to the finalizer"
keeper := {
   'hello' copy toFinalizeSend: #add: to: receiver with: 'first hello finalized'.
   'hello' copy toFinalizeSend: #add: to: receiver with: 'second hello finalized'.
self assert: receiver size = 0 description: 'no object copy should have been finalized yet'.
keeper := nil.
Smalltalk garbageCollect.
self assert: receiver size = 2 description: 'each object copy should have been finalized.'.
nicolas cellier   
09-25-08 23:46   
I find your code very good!

My answer developped at: [^]
is in FinalizationRegistry_KeyIdentity_Test_M6347-nice.2.cs