|Anonymous | Login||09-24-2020 08:56 UTC|
|Main | My View | View Issues | Change Log | Docs|
|Viewing Issue Advanced Details [ Jump to Notes ]||[ View Simple ] [ Issue History ] [ Print ]|
|ID||Category||Severity||Reproducibility||Date Submitted||Last Update|
|0007352||[Squeak] Kernel||major||random||05-13-09 13:53||02-06-11 23:48|
|Assigned To||nicolas cellier|
|ETA||none||Fixed in Version||trunk||Product Version||3.10.2bc|
|Summary||0007352: WeakMessageSend generates a DNU because of the message receiver being collected and disappearing|
|Description||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.|
|Steps To Reproduce|
|Additional Information||The problem happens in Squeak 3.10, Pharo-10243, and Cuis.|
WeakMessageSendFixPart1-jmv-M7352.cs [^] (865 bytes) 05-13-09 14:43
WeakMessageSendFixPart2-jmv-M7352.cs [^] (5,502 bytes) 05-13-09 14:44
WeakMessageSendFixPart1-jmv-M7352.9.cs [^] (1,055 bytes) 05-14-09 15:11
WeakMessageSendFixPart2-jmv-M7352.8.cs [^] (6,380 bytes) 05-14-09 15:12
(0013126 - 1521 - 1635 - 1635 - 1635 - 1635 - 1635)
Hi Juan, I approve the essential of the fix, however there are some details to be reviewed, so here are my code critics.
Changing this piece of art is not that straight forward!
1) #isReceiverOrAnyArgumentGarbage is a message only in WeakMessageSend
#isValid was also understood by MessageSend.
Now, with new definition of WeakActionSequence>>#asMinimalRepresentation, a WeakActionSequence cannot include a mixture of MessageSend and WeakMessageSend.
In old time, it could have. Is it on purpose?
2) Catching Exception (WeakActionSequenceTrappingError) is considered bad and dangerous, so watch your steps. You should maybe consider catching Error instead.
3) You have broken WeakActionSequence>>#value, because it will now return nil when last action is garbaged. You can't rely on using #asMinimalRepresentation previously because garbage collection can occur in between as you noticed. I don't know if this has any implication, but the API is clearly changed.
4) Beware, #isValid has another definition in WeakMessageSend subclass: #NonReentrantWeakMessageSend.
With your new usage of #asMinimalRepresentation, it would have been wrong to use NonReentrantWeakMessageSend.>>#isValid because it would exclude definitely a temporarily unavailable action... Unfortunately that also excludes MessageSend - see 1)
Beware if you correct 3), then a NonReentrantWeakMessageSend that is not valid but is not garbaged will return nil when evaluated, and this will be assigned to answer when it should not...
(0013127 - 582 - 634 - 634 - 634 - 634 - 634)
Hi Nicolas, thanks for reviewing!
1) Fixed. Added #isReceiverOrAnyArgumentGarbage to MessageSend. Did not add back #isValid because it is "false polymorphism", a message implemented by objects that are not polymorphic at all. This is bad in general, it adds confusion and fills 'implementors' and 'senders' with irrelevant stuff.
2) That problem was already there before. I fixed it.
4) WeakMessageSend does not have any subclass in stock 3.10. It seems that those are part of the PolyMorph package. I guess its mainainers need to update this. I did not fix this.
(0013142 - 596 - 638 - 638 - 638 - 638 - 638)
In terms of the semantics of NonReentrantWeakMessageSend, #isValid is not really necessary... the rentrancy is protected upon the call to #valueWithArguments:.
I'll remove in next update.
As for #valueWithEnoughArguments: it *could* be sent in our 3.9 images, just happens not to be used (only used with MessageSend in that image).
I can remove those from Polymorph too, with the caveat that they shouldn't be called in older images.
Of course, when a block is used as an action there's a problem (defines #isValid but not #isReceiverOrAnyArgumentGarbage). Then there's BlockClosures...
(0013843 - 314 - 356 - 806 - 806 - 806 - 806)
Note I did not remove WeakMessageSend>>valueWithEnoughArguments: though it is not used with such receiver.
Who knows, a package might need it...
|05-13-09 13:53||jmvuletich||New Issue|
|05-13-09 13:55||jmvuletich||File Added: 7352-WeakMessageSendFixPart1-jmv.5.cs|
|05-13-09 13:55||jmvuletich||File Added: 7352-WeakMessageSendFixPart2-jmv.4.cs|
|05-13-09 14:05||jmvuletich||Severity||minor => major|
|05-13-09 14:43||jmvuletich||File Deleted: 7352-WeakMessageSendFixPart1-jmv.5.cs|
|05-13-09 14:43||jmvuletich||File Deleted: 7352-WeakMessageSendFixPart2-jmv.4.cs|
|05-13-09 14:43||jmvuletich||File Added: WeakMessageSendFixPart1-jmv-M7352.cs|
|05-13-09 14:44||jmvuletich||File Added: WeakMessageSendFixPart2-jmv-M7352.cs|
|05-13-09 22:11||nicolas cellier||Note Added: 0013126|
|05-14-09 15:11||jmvuletich||File Added: WeakMessageSendFixPart1-jmv-M7352.9.cs|
|05-14-09 15:12||jmvuletich||File Added: WeakMessageSendFixPart2-jmv-M7352.8.cs|
|05-14-09 15:17||jmvuletich||Note Added: 0013127|
|07-02-09 12:42||GazzaGuru||Note Added: 0013142|
|08-22-10 19:43||nicolas cellier||Status||new => resolved|
|08-22-10 19:43||nicolas cellier||Fixed in Version||=> trunk|
|08-22-10 19:43||nicolas cellier||Resolution||open => fixed|
|08-22-10 19:43||nicolas cellier||Assigned To||=> nicolas cellier|
|08-22-10 19:43||nicolas cellier||Note Added: 0013843|
|02-06-11 23:48||leves||Status||resolved => closed|
| Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
68 total queries executed.|
40 unique queries executed.