Don't rely on weak reference cleanup for lookup of undeclared references

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Don't rely on weak reference cleanup for lookup of undeclared references

Max Leske
Hi,

Once again, there's a situation in the Fuel test suite that uncovered an issue. Consider the following script:

Object subclass: #Foo
instanceVariableNames: ''
classVariableNames: ''
poolDictionaries: ''
category: 'Kernel-Classes'.
10 timesRepeat: [(Smalltalk at: #Foo) rename: #Bar.
(Smalltalk at: #Bar) rename: #Foo ]

In 5.2-alpha this will open a warning dialog, as at some point one of the class names will be undeclared. Class>>rename: only checks for an entry in the undeclared dictionary without considering that the weak references may not yet have been cleaned up.

I understand that this is not really a realistic real world use-case but it's still annoying for tests. My current workaround is a forced GC after *every* test in the entire suite (I use ClassFactoryForTestCase, so class names may be reused many times over). Luckily, the impact on runtime isn't too bad.

I'd appreciate it if this could be fixed. I'd be even more happy if I could be notified of the fix, so that I can remove the workaround again.


Cheers,
Max


Reply | Threaded
Open this post in threaded view
|

Re: Don't rely on weak reference cleanup for lookup of undeclared references

Levente Uzonyi
On Sun, 27 May 2018, Max Leske wrote:

> Hi,
> Once again, there's a situation in the Fuel test suite that uncovered an issue. Consider the following script:
>
> Object subclass: #Foo
> instanceVariableNames: ''
> classVariableNames: ''
> poolDictionaries: ''
> category: 'Kernel-Classes'.
> 10 timesRepeat: [(Smalltalk at: #Foo) rename: #Bar.
> (Smalltalk at: #Bar) rename: #Foo ]
>
> In 5.2-alpha this will open a warning dialog, as at some point one of the class names will be undeclared. Class>>rename: only checks for an entry in the undeclared dictionary without considering that the weak
> references may not yet have been cleaned up.
>
> I understand that this is not really a realistic real world use-case but it's still annoying for tests. My current workaround is a forced GC after *every* test in the entire suite (I use
> ClassFactoryForTestCase, so class names may be reused many times over). Luckily, the impact on runtime isn't too bad.

Did you consider extending ClassFactoryForTestCase with an optional infix
name part for the generated names?
For example:

initialize

  super initialize.
  infix := ''.
  self createdClasses: IdentitySet new

infix: aString

  infix := aString

newName

  ^'ClassForTestToBeDeleted{1}{2}' format: {
  infix.
  self createdClasses size + 1 }

The you could specify a random infix, like UUID new asString36 to avoid
collisions.

Levente

>
> I'd appreciate it if this could be fixed. I'd be even more happy if I could be notified of the fix, so that I can remove the workaround again.
>
>
> Cheers,
> Max
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Don't rely on weak reference cleanup for lookup of undeclared references

Max Leske
In reply to this post by Max Leske
Hi Levente,


On 28 May 2018 at 13:06:46, [hidden email] ([hidden email]) wrote:

> Hi,
> Once again, there's a situation in the Fuel test suite that uncovered an issue. Consider the following script:
> 
> Object subclass: #Foo
> instanceVariableNames: ''
> classVariableNames: ''
> poolDictionaries: ''
> category: 'Kernel-Classes'.
> 10 timesRepeat: [(Smalltalk at: #Foo) rename: #Bar.
> (Smalltalk at: #Bar) rename: #Foo ]
> 
> In 5.2-alpha this will open a warning dialog, as at some point one of the class names will be undeclared. Class>>rename: only checks for an entry in the undeclared dictionary without considering that the weak
> references may not yet have been cleaned up.
> 
> I understand that this is not really a realistic real world use-case but it's still annoying for tests. My current workaround is a forced GC after *every* test in the entire suite (I use
> ClassFactoryForTestCase, so class names may be reused many times over). Luckily, the impact on runtime isn't too bad.

Did you consider extending ClassFactoryForTestCase with an optional infix 
name part for the generated names?
For example:

initialize

super initialize.
infix := ''.
self createdClasses: IdentitySet new

infix: aString

infix := aString

newName

^'ClassForTestToBeDeleted{1}{2}' format: {
infix.
self createdClasses size + 1 }

The you could specify a random infix, like UUID new asString36 to avoid 
collisions.

Levente

Yes, I did consider that. However, in my opinion it should not be the users concern. Using ClassFactoryForTestCase should in itself be sufficient to guarantee the independence of tests. I knew what I was looking for, but for someone with less experience this would just be a very annoying Heisenbug.

Cheers,

Max



> 
> I'd appreciate it if this could be fixed. I'd be even more happy if I could be notified of the fix, so that I can remove the workaround again.
> 
> 
> Cheers,
> Max
> 
>


Reply | Threaded
Open this post in threaded view
|

Re: Don't rely on weak reference cleanup for lookup of undeclared references

Eliot Miranda-2
In reply to this post by Max Leske
Hi Max,


On May 27, 2018, at 2:44 PM, Max Leske <[hidden email]> wrote:

Hi,

Once again, there's a situation in the Fuel test suite that uncovered an issue. Consider the following script:

Object subclass: #Foo
instanceVariableNames: ''
classVariableNames: ''
poolDictionaries: ''
category: 'Kernel-Classes'.
10 timesRepeat: [(Smalltalk at: #Foo) rename: #Bar.
(Smalltalk at: #Bar) rename: #Foo ]

In 5.2-alpha this will open a warning dialog, as at some point one of the class names will be undeclared. Class>>rename: only checks for an entry in the undeclared dictionary without considering that the weak references may not yet have been cleaned up.

I understand that this is not really a realistic real world use-case but it's still annoying for tests. My current workaround is a forced GC after *every* test in the entire suite (I use ClassFactoryForTestCase, so class names may be reused many times over). Luckily, the impact on runtime isn't too bad.

I'd appreciate it if this could be fixed. I'd be even more happy if I could be notified of the fix, so that I can remove the workaround again.

Do you have ideas about where the bug lies?



Cheers,
Max


_,,,^..^,,,_ (phone)


Reply | Threaded
Open this post in threaded view
|

Re: Don't rely on weak reference cleanup for lookup of undeclared references

Max Leske
In reply to this post by Max Leske
Hi Eliot,


 

On 28 May 2018 at 22:03:24, [hidden email] ([hidden email]) wrote:

Hi Max,


> <a href="http://airmail.calendar/2018-05-27 14:44:00 CEST" style="font-family:helvetica;font-size:13px;font-style:normal;font-variant-caps:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On May 27, 2018, at 2:44 PM, Max Leske <[hidden email]> wrote:
> 
> Hi,
> 
> Once again, there's a situation in the Fuel test suite that uncovered an issue. Consider the following script:
> 
> Object subclass: #Foo
> instanceVariableNames: ''
> classVariableNames: ''
> poolDictionaries: ''
> category: 'Kernel-Classes'.
>
> 10 timesRepeat: [(Smalltalk at: #Foo) rename: #Bar.
> (Smalltalk at: #Bar) rename: #Foo ]
> 
> In 5.2-alpha this will open a warning dialog, as at some point one of the class names will be undeclared. Class>>rename: only checks for an entry in the undeclared dictionary without considering that the weak references may not yet have been cleaned up.
> 
> I understand that this is not really a realistic real world use-case but it's still annoying for tests. My current workaround is a forced GC after *every* test in the entire suite (I use ClassFactoryForTestCase, so class names may be reused many times over). Luckily, the impact on runtime isn't too bad.
> 
> I'd appreciate it if this could be fixed. I'd be even more happy if I could be notified of the fix, so that I can remove the workaround again.

Do you have ideas about where the bug lies?

Sort of. In Pharo it seems that when renaming a class, the original class name is never added to Undeclared while in Squeak it is. 

It looks to me, that the problem is that Environment>>unbind: is never sent. In Environment>>renameClass:from:to: #binding:removedFrom: undeclares the original class name and #binding:addedTo: adds the new binding to the environment. But the now undeclared original class name remains undeclared. The following modification gets rid of the problem:

<snip>

aClass updateMethodBindingsTo: newBinding.

declarations add: newBinding. 

self binding: newBinding addedTo: self.

self unbind: oldBinding key. "<---------------------- added"

<snip>


HTH,

Max




> 
> Cheers,
> Max


Reply | Threaded
Open this post in threaded view
|

Re: Don't rely on weak reference cleanup for lookup of undeclared references

Max Leske
In reply to this post by Max Leske
Bump.


> Hi Eliot,
>
>
>  
>
> On 28 May 2018 at 22:03:24,
> [hidden email]
> ([hidden email]) wrote:
>
> Hi Max,
>
>
>>  On May 27, 2018, at 2:44 PM, Max Leske <[hidden email]> wrote:
>>  
>> Hi,
>>  
>> Once again, there's a situation in the Fuel test suite that uncovered
>> an issue. Consider the following script:
>>  
>> Object subclass: #Foo
>> instanceVariableNames: ''
>> classVariableNames: ''
>> poolDictionaries: ''
>> category: 'Kernel-Classes'.
>>
>> 10 timesRepeat: [(Smalltalk at: #Foo) rename: #Bar.
>> (Smalltalk at: #Bar) rename: #Foo ]
>>  
>> In 5.2-alpha this will open a warning dialog, as at some point one of
>> the class names will be undeclared. Class>>rename: only checks for an
>> entry in the undeclared dictionary without considering that the weak
>> references may not yet have been cleaned up.
>>  
>> I understand that this is not really a realistic real world use-case
>> but it's still annoying for tests. My current workaround is a forced
>> GC after *every* test in the entire suite (I use
>> ClassFactoryForTestCase, so class names may be reused many times
>> over). Luckily, the impact on
>> runtime isn't too bad.
>>  
>> I'd appreciate it if this could be fixed. I'd be even more happy if I
>> could be notified of the fix, so that I can remove the workaround
>> again.
>
> Do you have ideas about where the bug lies?
> Sort of. In Pharo it seems that when renaming a class, the original
> class name is never added to Undeclared while in Squeak it is. 
>
> It looks to me, that the problem is that Environment>>unbind: is never
> sent. In Environment>>renameClass:from:to: #binding:removedFrom:
> undeclares the original class name and #binding:addedTo: adds the new
> binding to the environment. But the now undeclared original class name
> remains undeclared. The following modification gets rid of the
> problem:
>
> <snip>
>
> aClass updateMethodBindingsTo: newBinding.
>
> declarations add: newBinding. 
>
> self binding: newBinding addedTo: self.
>
> self unbind: oldBinding key. "<---------------------- added"
>
> <snip>
>
>
>
> HTH,
>
> Max
>
>
>
>
>>  
>> Cheers,
>> Max

Reply | Threaded
Open this post in threaded view
|

Re: Don't rely on weak reference cleanup for lookup of undeclared references

Levente Uzonyi
Pharo uses RB to rename classes. Squeak has not integrated that to the
Trunk, so it is not possible to rewrite code using the renamed class just
yet, therefore the old binding is kept and stored in Undeclared.

Levente

On Tue, 5 Jun 2018, Max Leske wrote:

> Bump.
>
>
>> Hi Eliot,
>>
>>
>>  
>>
>> On 28 May 2018 at 22:03:24,
>> [hidden email]
>> ([hidden email]) wrote:
>>
>> Hi Max,
>>
>>
>>>  On May 27, 2018, at 2:44 PM, Max Leske <[hidden email]> wrote:
>>>  
>>> Hi,
>>>  
>>> Once again, there's a situation in the Fuel test suite that uncovered
>>> an issue. Consider the following script:
>>>  
>>> Object subclass: #Foo
>>> instanceVariableNames: ''
>>> classVariableNames: ''
>>> poolDictionaries: ''
>>> category: 'Kernel-Classes'.
>>>
>>> 10 timesRepeat: [(Smalltalk at: #Foo) rename: #Bar.
>>> (Smalltalk at: #Bar) rename: #Foo ]
>>>  
>>> In 5.2-alpha this will open a warning dialog, as at some point one of
>>> the class names will be undeclared. Class>>rename: only checks for an
>>> entry in the undeclared dictionary without considering that the weak
>>> references may not yet have been cleaned up.
>>>  
>>> I understand that this is not really a realistic real world use-case
>>> but it's still annoying for tests. My current workaround is a forced
>>> GC after *every* test in the entire suite (I use
>>> ClassFactoryForTestCase, so class names may be reused many times
>>> over). Luckily, the impact on
>>> runtime isn't too bad.
>>>  
>>> I'd appreciate it if this could be fixed. I'd be even more happy if I
>>> could be notified of the fix, so that I can remove the workaround
>>> again.
>>
>> Do you have ideas about where the bug lies?
>> Sort of. In Pharo it seems that when renaming a class, the original
>> class name is never added to Undeclared while in Squeak it is. 
>>
>> It looks to me, that the problem is that Environment>>unbind: is never
>> sent. In Environment>>renameClass:from:to: #binding:removedFrom:
>> undeclares the original class name and #binding:addedTo: adds the new
>> binding to the environment. But the now undeclared original class name
>> remains undeclared. The following modification gets rid of the
>> problem:
>>
>> <snip>
>>
>> aClass updateMethodBindingsTo: newBinding.
>>
>> declarations add: newBinding. 
>>
>> self binding: newBinding addedTo: self.
>>
>> self unbind: oldBinding key. "<---------------------- added"
>>
>> <snip>
>>
>>
>>
>> HTH,
>>
>> Max
>>
>>
>>
>>
>>>  
>>> Cheers,
>>> Max
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Don't rely on weak reference cleanup for lookup of undeclared references

Levente Uzonyi
Well, Pharo still has #rename: implemented on Class, but it will simply
break your source code if you use it. Because it'll change the binding to
the new one, but keep the source code as it was before renaming.

Levente

On Tue, 5 Jun 2018, Levente Uzonyi wrote:

> Pharo uses RB to rename classes. Squeak has not integrated that to the Trunk,
> so it is not possible to rewrite code using the renamed class just yet,
> therefore the old binding is kept and stored in Undeclared.
>
> Levente
>
> On Tue, 5 Jun 2018, Max Leske wrote:
>
>> Bump.
>>
>>
>>> Hi Eliot,
>>>
>>>
>>>  
>>>
>>> On 28 May 2018 at 22:03:24, [hidden email]
>>> ([hidden email]) wrote:
>>>
>>> Hi Max,
>>>
>>>
>>>>  On May 27, 2018, at 2:44 PM, Max Leske <[hidden email]> wrote:
>>>>  
>>>> Hi,
>>>>  
>>>> Once again, there's a situation in the Fuel test suite that uncovered an
>>>> issue. Consider the following script:
>>>>  
>>>> Object subclass: #Foo
>>>> instanceVariableNames: ''
>>>> classVariableNames: ''
>>>> poolDictionaries: ''
>>>> category: 'Kernel-Classes'.
>>>>
>>>> 10 timesRepeat: [(Smalltalk at: #Foo) rename: #Bar.
>>>> (Smalltalk at: #Bar) rename: #Foo ]
>>>>  
>>>> In 5.2-alpha this will open a warning dialog, as at some point one of the
>>>> class names will be undeclared. Class>>rename: only checks for an entry
>>>> in the undeclared dictionary without considering that the weak references
>>>> may not yet have been cleaned up.
>>>>  
>>>> I understand that this is not really a realistic real world use-case but
>>>> it's still annoying for tests. My current workaround is a forced GC after
>>>> *every* test in the entire suite (I use ClassFactoryForTestCase, so class
>>>> names may be reused many times over). Luckily, the impact on
>>>> runtime isn't too bad.
>>>>  
>>>> I'd appreciate it if this could be fixed. I'd be even more happy if I
>>>> could be notified of the fix, so that I can remove the workaround again.
>>>
>>> Do you have ideas about where the bug lies?
>>> Sort of. In Pharo it seems that when renaming a class, the original class
>>> name is never added to Undeclared while in Squeak it is. 
>>>
>>> It looks to me, that the problem is that Environment>>unbind: is never
>>> sent. In Environment>>renameClass:from:to: #binding:removedFrom:
>>> undeclares the original class name and #binding:addedTo: adds the new
>>> binding to the environment. But the now undeclared original class name
>>> remains undeclared. The following modification gets rid of the problem:
>>>
>>> <snip>
>>>
>>> aClass updateMethodBindingsTo: newBinding.
>>>
>>> declarations add: newBinding. 
>>>
>>> self binding: newBinding addedTo: self.
>>>
>>> self unbind: oldBinding key. "<---------------------- added"
>>>
>>> <snip>
>>>
>>>
>>>
>>> HTH,
>>>
>>> Max
>>>
>>>
>>>
>>>
>>>>  
>>>> Cheers,
>>>> Max
>>
>