[squeak-dev] [BUG][FIX] Bug in WeakMessageSend
Locked
[squeak-dev] [BUG][FIX] Bug in WeakMessageSend
 
|
Hi Folks,
I found a bug in WeakMessageSend that generates a DNU because of the message receiver being collected and disappearing. The culprits are #ensureReceiver and #ensureReceiverAndArguments . These methods ensure nothing. It might happen (although not often) that the receiver or some argument is collected after this method is called, but before it is actually used. The problem happens in Squeak 3.10, Pharo-10243, and Cuis. Please take a look. Also take a look at my proposed solution. I'm not sure, maybe some will object keeping an artificial reference to an object that could have been collected otherwise, eve if it is just for the time of a message send... Anyway, comments welcome. Cheers, Juan Vuletich 'From Squeak3.7 of ''4 September 2004'' [latest update: #5989] on 12 May 2009 at 11:02:25 am'! !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 10:59'! value ^ arguments isNil ifTrue: [ self withEnsuredReceiverDo: [ :r | r perform: selector]] ifFalse: [ self withEnsuredReceiverAndArgumentsDo: [ :r :a | r perform: selector withArguments: a]]! ! !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:00'! valueWithArguments: anArray | argsToUse | "Safe to use, because they are built before ensureing receiver and args..." argsToUse _ self collectArguments: anArray. ^self withEnsuredReceiverAndArgumentsDo: [ :r :a | r perform: selector withArguments: argsToUse ]! ! !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:01'! valueWithEnoughArguments: anArray "call the selector with enough arguments from arguments and anArray" | args | args _ Array new: selector numArgs. args replaceFrom: 1 to: ( arguments size min: args size) with: arguments startingAt: 1. args size > arguments size ifTrue: [ args replaceFrom: arguments size + 1 to: (arguments size + anArray size min: args size) with: anArray startingAt: 1. ]. "args is safe to use, because they are built before ensureing receiver and args..." ^self withEnsuredReceiverAndArgumentsDo: [ :r :a | r perform: selector withArguments: args ]! ! !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:54'! withEnsuredReceiverAndArgumentsDo: aBlock "Grab real references to receiver and arguments. If they still exist, evaluate aBlock." "Return if my receiver has gone away" | r a | r _ self receiver. r ifNil: [ ^self ]. "Make sure that my arguments haven't gone away" "arguments can not become nil. The extra care in #ensureReceiverAndArguments is wrong (BTW, the whole idea is wrong, that's why we do these new methods...)" a _ Array withAll: arguments. a with: shouldBeNil do: [ :arg :flag | arg ifNil: [ flag ifFalse: [ ^self ]] ]. aBlock value: r value: a! ! !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:53'! withEnsuredReceiverDo: aBlock "Grab a real reference to receive. If still there, evaluate aBlock." "Return if my receiver has gone away" | r | r _ self receiver. r ifNil: [ ^self ]. aBlock value: r! ! WeakMessageSend removeSelector: #ensureReceiver! WeakMessageSend removeSelector: #ensureReceiverAndArguments! |
Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend
|
|
Yes sure, that was stupid!
But: 1) #value & al used to return the value of evaluating the message, or nil if any receiver/argument garbaged. Now #value & al will always return self 2) #valueWithArguments: should not care if arguments were garbage collected, because some replacement arguments were provided 3) #valueWithEnoughArguments: should care of garbage collected arguments from: anArray size to: arguments size. Please publish a mantis report, then upload your updated code :) Cheers Nicolas 2009/5/12 Juan Vuletich <[hidden email]>: > Hi Folks, > > I found a bug in WeakMessageSend that generates a DNU because of the message > receiver being collected and disappearing. The culprits are #ensureReceiver > and #ensureReceiverAndArguments . These methods ensure nothing. It might > happen (although not often) that the receiver or some argument is collected > after this method is called, but before it is actually used. > > The problem happens in Squeak 3.10, Pharo-10243, and Cuis. > > Please take a look. Also take a look at my proposed solution. I'm not sure, > maybe some will object keeping an artificial reference to an object that > could have been collected otherwise, eve if it is just for the time of a > message send... Anyway, comments welcome. > > Cheers, > Juan Vuletich > > 'From Squeak3.7 of ''4 September 2004'' [latest update: #5989] on 12 May > 2009 at 11:02:25 am'! > > !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 10:59'! > value > ^ arguments isNil > ifTrue: [ > self withEnsuredReceiverDo: [ :r | r perform: > selector]] > ifFalse: [ > self withEnsuredReceiverAndArgumentsDo: [ :r :a | > r > perform: selector > withArguments: a]]! ! > > !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:00'! > valueWithArguments: anArray > | argsToUse | > > "Safe to use, because they are built before ensureing receiver and > args..." > argsToUse _ self collectArguments: anArray. > ^self withEnsuredReceiverAndArgumentsDo: [ :r :a | > r > perform: selector > withArguments: argsToUse ]! ! > > !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:01'! > valueWithEnoughArguments: anArray > > "call the selector with enough arguments from arguments and anArray" > | args | > args _ Array new: selector numArgs. > args replaceFrom: 1 > to: ( arguments size min: args size) > with: arguments > startingAt: 1. > args size > arguments size ifTrue: [ > args replaceFrom: arguments size + 1 > to: (arguments size + anArray size min: args size) > with: anArray > startingAt: 1. > ]. > > "args is safe to use, because they are built before ensureing > receiver and args..." > ^self withEnsuredReceiverAndArgumentsDo: [ :r :a | > r > perform: selector > withArguments: args ]! ! > > !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:54'! > withEnsuredReceiverAndArgumentsDo: aBlock > "Grab real references to receiver and arguments. If they still exist, > evaluate aBlock." > > "Return if my receiver has gone away" > | r a | > r _ self receiver. > r ifNil: [ ^self ]. > > > "Make sure that my arguments haven't gone away" > "arguments can not become nil. The extra care in > #ensureReceiverAndArguments is wrong > (BTW, the whole idea is wrong, that's why we do these new > methods...)" > a _ Array withAll: arguments. > a with: shouldBeNil do: [ :arg :flag | > arg ifNil: [ flag ifFalse: [ ^self ]] > ]. > > aBlock value: r value: a! ! > > !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:53'! > withEnsuredReceiverDo: aBlock > "Grab a real reference to receive. If still there, evaluate aBlock." > > "Return if my receiver has gone away" > | r | > r _ self receiver. > r ifNil: [ ^self ]. > > aBlock value: r! ! > > WeakMessageSend removeSelector: #ensureReceiver! > WeakMessageSend removeSelector: #ensureReceiverAndArguments! > > > > |
Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend
|
|
Oops I did not understand the API at first glance...
3) was wrong because arguments are added first, and anArray last - One more thing: WeakActionSequence using isValid test is fragile too... 2009/5/12 Nicolas Cellier <[hidden email]>: > Yes sure, that was stupid! > > But: > > 1) #value & al used to return the value of evaluating the message, or > nil if any receiver/argument garbaged. > Now #value & al will always return self > > 2) #valueWithArguments: should not care if arguments were garbage > collected, because some replacement arguments were provided > > 3) #valueWithEnoughArguments: should care of garbage collected > arguments from: anArray size to: arguments size. > > Please publish a mantis report, then upload your updated code :) > > Cheers > > Nicolas > > 2009/5/12 Juan Vuletich <[hidden email]>: >> Hi Folks, >> >> I found a bug in WeakMessageSend that generates a DNU because of the message >> receiver being collected and disappearing. The culprits are #ensureReceiver >> and #ensureReceiverAndArguments . These methods ensure nothing. It might >> happen (although not often) that the receiver or some argument is collected >> after this method is called, but before it is actually used. >> >> The problem happens in Squeak 3.10, Pharo-10243, and Cuis. >> >> Please take a look. Also take a look at my proposed solution. I'm not sure, >> maybe some will object keeping an artificial reference to an object that >> could have been collected otherwise, eve if it is just for the time of a >> message send... Anyway, comments welcome. >> >> Cheers, >> Juan Vuletich >> >> 'From Squeak3.7 of ''4 September 2004'' [latest update: #5989] on 12 May >> 2009 at 11:02:25 am'! >> >> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 10:59'! >> value >> ^ arguments isNil >> ifTrue: [ >> self withEnsuredReceiverDo: [ :r | r perform: >> selector]] >> ifFalse: [ >> self withEnsuredReceiverAndArgumentsDo: [ :r :a | >> r >> perform: selector >> withArguments: a]]! ! >> >> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:00'! >> valueWithArguments: anArray >> | argsToUse | >> >> "Safe to use, because they are built before ensureing receiver and >> args..." >> argsToUse _ self collectArguments: anArray. >> ^self withEnsuredReceiverAndArgumentsDo: [ :r :a | >> r >> perform: selector >> withArguments: argsToUse ]! ! >> >> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:01'! >> valueWithEnoughArguments: anArray >> >> "call the selector with enough arguments from arguments and anArray" >> | args | >> args _ Array new: selector numArgs. >> args replaceFrom: 1 >> to: ( arguments size min: args size) >> with: arguments >> startingAt: 1. >> args size > arguments size ifTrue: [ >> args replaceFrom: arguments size + 1 >> to: (arguments size + anArray size min: args size) >> with: anArray >> startingAt: 1. >> ]. >> >> "args is safe to use, because they are built before ensureing >> receiver and args..." >> ^self withEnsuredReceiverAndArgumentsDo: [ :r :a | >> r >> perform: selector >> withArguments: args ]! ! >> >> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:54'! >> withEnsuredReceiverAndArgumentsDo: aBlock >> "Grab real references to receiver and arguments. If they still exist, >> evaluate aBlock." >> >> "Return if my receiver has gone away" >> | r a | >> r _ self receiver. >> r ifNil: [ ^self ]. >> >> >> "Make sure that my arguments haven't gone away" >> "arguments can not become nil. The extra care in >> #ensureReceiverAndArguments is wrong >> (BTW, the whole idea is wrong, that's why we do these new >> methods...)" >> a _ Array withAll: arguments. >> a with: shouldBeNil do: [ :arg :flag | >> arg ifNil: [ flag ifFalse: [ ^self ]] >> ]. >> >> aBlock value: r value: a! ! >> >> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:53'! >> withEnsuredReceiverDo: aBlock >> "Grab a real reference to receive. If still there, evaluate aBlock." >> >> "Return if my receiver has gone away" >> | r | >> r _ self receiver. >> r ifNil: [ ^self ]. >> >> aBlock value: r! ! >> >> WeakMessageSend removeSelector: #ensureReceiver! >> WeakMessageSend removeSelector: #ensureReceiverAndArguments! >> >> >> >> > |
Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend
|
|
Oh, and I forgot, correct class comment:
shouldBeNil Array of: Boolean -- used to distinguish nil arguments from garbage collected arguments 2009/5/12 Nicolas Cellier <[hidden email]>: > Oops I did not understand the API at first glance... > 3) was wrong because arguments are added first, and anArray last - > > One more thing: WeakActionSequence using isValid test is fragile too... > > > 2009/5/12 Nicolas Cellier <[hidden email]>: >> Yes sure, that was stupid! >> >> But: >> >> 1) #value & al used to return the value of evaluating the message, or >> nil if any receiver/argument garbaged. >> Now #value & al will always return self >> >> 2) #valueWithArguments: should not care if arguments were garbage >> collected, because some replacement arguments were provided >> >> 3) #valueWithEnoughArguments: should care of garbage collected >> arguments from: anArray size to: arguments size. >> >> Please publish a mantis report, then upload your updated code :) >> >> Cheers >> >> Nicolas >> >> 2009/5/12 Juan Vuletich <[hidden email]>: >>> Hi Folks, >>> >>> I found a bug in WeakMessageSend that generates a DNU because of the message >>> receiver being collected and disappearing. The culprits are #ensureReceiver >>> and #ensureReceiverAndArguments . These methods ensure nothing. It might >>> happen (although not often) that the receiver or some argument is collected >>> after this method is called, but before it is actually used. >>> >>> The problem happens in Squeak 3.10, Pharo-10243, and Cuis. >>> >>> Please take a look. Also take a look at my proposed solution. I'm not sure, >>> maybe some will object keeping an artificial reference to an object that >>> could have been collected otherwise, eve if it is just for the time of a >>> message send... Anyway, comments welcome. >>> >>> Cheers, >>> Juan Vuletich >>> >>> 'From Squeak3.7 of ''4 September 2004'' [latest update: #5989] on 12 May >>> 2009 at 11:02:25 am'! >>> >>> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 10:59'! >>> value >>> ^ arguments isNil >>> ifTrue: [ >>> self withEnsuredReceiverDo: [ :r | r perform: >>> selector]] >>> ifFalse: [ >>> self withEnsuredReceiverAndArgumentsDo: [ :r :a | >>> r >>> perform: selector >>> withArguments: a]]! ! >>> >>> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:00'! >>> valueWithArguments: anArray >>> | argsToUse | >>> >>> "Safe to use, because they are built before ensureing receiver and >>> args..." >>> argsToUse _ self collectArguments: anArray. >>> ^self withEnsuredReceiverAndArgumentsDo: [ :r :a | >>> r >>> perform: selector >>> withArguments: argsToUse ]! ! >>> >>> !WeakMessageSend methodsFor: 'evaluating' stamp: 'jmv 5/12/2009 11:01'! >>> valueWithEnoughArguments: anArray >>> >>> "call the selector with enough arguments from arguments and anArray" >>> | args | >>> args _ Array new: selector numArgs. >>> args replaceFrom: 1 >>> to: ( arguments size min: args size) >>> with: arguments >>> startingAt: 1. >>> args size > arguments size ifTrue: [ >>> args replaceFrom: arguments size + 1 >>> to: (arguments size + anArray size min: args size) >>> with: anArray >>> startingAt: 1. >>> ]. >>> >>> "args is safe to use, because they are built before ensureing >>> receiver and args..." >>> ^self withEnsuredReceiverAndArgumentsDo: [ :r :a | >>> r >>> perform: selector >>> withArguments: args ]! ! >>> >>> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:54'! >>> withEnsuredReceiverAndArgumentsDo: aBlock >>> "Grab real references to receiver and arguments. If they still exist, >>> evaluate aBlock." >>> >>> "Return if my receiver has gone away" >>> | r a | >>> r _ self receiver. >>> r ifNil: [ ^self ]. >>> >>> >>> "Make sure that my arguments haven't gone away" >>> "arguments can not become nil. The extra care in >>> #ensureReceiverAndArguments is wrong >>> (BTW, the whole idea is wrong, that's why we do these new >>> methods...)" >>> a _ Array withAll: arguments. >>> a with: shouldBeNil do: [ :arg :flag | >>> arg ifNil: [ flag ifFalse: [ ^self ]] >>> ]. >>> >>> aBlock value: r value: a! ! >>> >>> !WeakMessageSend methodsFor: 'private' stamp: 'jmv 5/12/2009 10:53'! >>> withEnsuredReceiverDo: aBlock >>> "Grab a real reference to receive. If still there, evaluate aBlock." >>> >>> "Return if my receiver has gone away" >>> | r | >>> r _ self receiver. >>> r ifNil: [ ^self ]. >>> >>> aBlock value: r! ! >>> >>> WeakMessageSend removeSelector: #ensureReceiver! >>> WeakMessageSend removeSelector: #ensureReceiverAndArguments! >>> >>> >>> >>> >> > |
Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend
|
|
Done. Mantis issue 7352. Please take a look at updated code.
Comments interleaved... Nicolas Cellier wrote: > Oh, and I forgot, correct class comment: > > shouldBeNil Array of: Boolean -- used to distinguish nil arguments > from garbage collected arguments > > added > 2009/5/12 Nicolas Cellier <[hidden email]>: > >> Oops I did not understand the API at first glance... >> 3) was wrong because arguments are added first, and anArray last - >> >> One more thing: WeakActionSequence using isValid test is fragile too... >> >> It actually was not fragile, becuse it used the new safe methods in WeakMessageSend. What I did is to remove the #isValid tests. >> 2009/5/12 Nicolas Cellier <[hidden email]>: >> >>> Yes sure, that was stupid! >>> >>> But: >>> >>> 1) #value & al used to return the value of evaluating the message, or >>> nil if any receiver/argument garbaged. >>> Now #value & al will always return self >>> fixed. thanks. >>> 2) #valueWithArguments: should not care if arguments were garbage >>> collected, because some replacement arguments were provided >>> What I did with this is to honor the old behavior. The (unwritten) specification is: "If the receiver or some argument is collected, then do nothing". Sounds reasonable, and I saw no reason to change this. >>> 3) #valueWithEnoughArguments: should care of garbage collected >>> arguments from: anArray size to: arguments size. >>> >>> Please publish a mantis report, then upload your updated code :) >>> >>> Cheers >>> >>> I also changed all _ to := , to ease integration in Croquet. Cheers, Juan Vuletich |
Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend
|
|
2009/5/13 Juan Vuletich <[hidden email]>:
> Done. Mantis issue 7352. Please take a look at updated code. > > Comments interleaved... > > Nicolas Cellier wrote: >> >> Oh, and I forgot, correct class comment: >> >> shouldBeNil Array of: Boolean -- used to distinguish nil >> arguments >> from garbage collected arguments >> >> > > added > >> 2009/5/12 Nicolas Cellier <[hidden email]>: >> >>> >>> Oops I did not understand the API at first glance... >>> 3) was wrong because arguments are added first, and anArray last - >>> >>> One more thing: WeakActionSequence using isValid test is fragile too... >>> >>> > > It actually was not fragile, becuse it used the new safe methods in > WeakMessageSend. What I did is to remove the #isValid tests. > >>> 2009/5/12 Nicolas Cellier <[hidden email]>: >>> >>>> >>>> Yes sure, that was stupid! >>>> >>>> But: >>>> >>>> 1) #value & al used to return the value of evaluating the message, or >>>> nil if any receiver/argument garbaged. >>>> Now #value & al will always return self >>>> > > fixed. thanks. > >>>> 2) #valueWithArguments: should not care if arguments were garbage >>>> collected, because some replacement arguments were provided >>>> > > What I did with this is to honor the old behavior. The (unwritten) > specification is: "If the receiver or some argument is collected, then do > nothing". Sounds reasonable, and I saw no reason to change this. > Sure, this is a very odd undocumented API, so it's hard to decide whether a bug or not. Usage should be analyzed first. My guess is that intention was to provide nil arguments where we don't want Weakness, and replace these nils further with provided replacement anArray... But the implementation is limited to these two cases: 1) Weak arguments are all at the beginning and we use #valueWithEnoughArguments: 2) Weak arguments are all at the end and the shouldBeNil are all at the beginning The case of a Weak argument superseded by an element of anArray should be a side effect hardly ever used, so I guess that does not much matter to keep or change API in this special case. In Pharo, I would recommend to deprecate and sweep this stuff (provide a better API if necessary). In distributions claiming maximum compatibility, I would recommend keeping the API. But it's not ideal to be stucked on such kind of code. Nicolas >>>> 3) #valueWithEnoughArguments: should care of garbage collected >>>> arguments from: anArray size to: arguments size. >>>> >>>> Please publish a mantis report, then upload your updated code :) >>>> >>>> Cheers >>>> >>>> > > I also changed all _ to := , to ease integration in Croquet. > > Cheers, > Juan Vuletich > > |
Attention Polymorph package maintainers - Re: [squeak-dev] [BUG][FIX] Bug in WeakMessageSend
|
|
In reply to this post by Juan Vuletich-4
Hi Nicolas,
I saw your notes to http://bugs.squeak.org/view.php?id=7352 . Thanks for your careful and detailed review! I addressed all the issues you raise with new change sets, except those about WeakMessageSend subclasses. It seems that NonReentrantWeakMessageSend and ExclusiveWeakMessageSend are part of the Polymorph package. I believe they should modify them as needed. Cheers, Juan Vuletich |
«
Return to Squeak - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |