Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0006347 [Squeak] Kernel minor always 03-18-07 13:42 09-25-08 23:46
Reporter loewis View Status public  
Assigned To
Priority normal Resolution open  
Status new   Product Version
Summary 0006347: [FIX] testFinalizationOfEquals fails
Description 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.
Additional Information
Attached Files  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

- Relationships

- Notes
(0010548 - 57 - 63 - 63 - 63 - 63 - 63)
edgardec
04-15-07 10:04

ObjectFinalizerTests don't fail in 3.10.
Some is wrong ?
 
(0010550 - 176 - 176 - 176 - 176 - 176 - 176)
loewis
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.
 
(0010552 - 98 - 116 - 116 - 116 - 116 - 116)
edgardec
04-15-07 20:20
edited on: 04-16-07 12:53

Well , yours is not a fix, is the old testFinalizationOfEquals in 3.9.
Ask Ralph for what to do

 
(0012305 - 502 - 548 - 548 - 548 - 548 - 548)
noha
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.
 
(0012687 - 673 - 751 - 751 - 751 - 751 - 751)
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!

Cheers
 
(0012688 - 772 - 1107 - 1107 - 1107 - 1107 - 1107)
nicolas cellier
09-23-08 20:40
edited on: 09-23-08 20:44

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;
        copy]
(#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.

 
(0012689 - 495 - 555 - 555 - 555 - 555 - 555)
noha
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.
 
(0012690 - 726 - 830 - 830 - 830 - 830 - 830)
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!).

Cheers
 
(0012691 - 790 - 974 - 974 - 974 - 974 - 974)
noha
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

self
      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.
 
(0012692 - 1368 - 1694 - 1694 - 1694 - 1694 - 1694)
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.
Cheers
 
(0012693 - 1275 - 1532 - 1532 - 1532 - 1532 - 1532)
noha
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.'.
 
(0012696 - 209 - 239 - 451 - 451 - 451 - 451)
nicolas cellier
09-25-08 23:46

I find your code very good!

My answer developped at: http://lists.squeakfoundation.org/pipermail/squeak-dev/2008-September/131654.html [^]
is in FinalizationRegistry_KeyIdentity_Test_M6347-nice.2.cs

Cheers
 

- Issue History
Date Modified Username Field Change
03-18-07 13:42 loewis New Issue
03-18-07 13:42 loewis File Added: oftfix.1.cs
03-20-07 23:02 loewis Issue Monitored: loewis
04-15-07 10:04 edgardec Note Added: 0010548
04-15-07 17:54 loewis Note Added: 0010550
04-15-07 20:20 edgardec Note Added: 0010552
04-16-07 12:53 edgardec Note Edited: 0010552
06-18-08 11:18 noha Note Added: 0012305
06-18-08 11:19 noha Issue Monitored: noha
09-23-08 20:04 nicolas cellier Note Added: 0012687
09-23-08 20:40 nicolas cellier Note Added: 0012688
09-23-08 20:44 nicolas cellier Note Edited: 0012688
09-23-08 20:44 nicolas cellier File Added: FinalizationRegistry_KeyIdentity_Test_M6347-nice.1.cs
09-24-08 08:40 noha Note Added: 0012689
09-24-08 16:37 nicolas cellier Note Added: 0012690
09-24-08 16:57 noha Note Added: 0012691
09-24-08 19:48 nicolas cellier File Added: FinalizationRegistry_KeyIdentity_Patch_M6347-nice.1.cs
09-24-08 21:16 nicolas cellier Note Added: 0012692
09-24-08 22:19 noha Note Added: 0012693
09-25-08 23:43 nicolas cellier File Added: FinalizationRegistry_KeyIdentity_Test_M6347-nice.2.cs
09-25-08 23:46 nicolas cellier Note Added: 0012696


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
96 total queries executed.
51 unique queries executed.
Powered by Mantis Bugtracker