Ben, This is an interesting idea - and an important topic. My main concern, that you seem to share, is that the default case should be as fast as possible and free of locking, as much as possible. (For example, I consider the #mutexForPicking used by #atRandom on Collection to be very silly, since normal access does nothing at all). Have you tried what would happen if you added your safety mechanism ? Would it get tripped in various places already ? Sven PS: BTW, thank you for your enthusiasm for tackling so many difficult/dangerous topics !
On 06 Dec 2014, at 07:10, Ben Coman <btc@openInWorld.com> wrote:
_BACKGROUND_
In relation to... https://pharo.fogbugz.com/default.asp?13182 "Refactor MCFileRepositoryInspector >> versionSearchAccept:"
I am reviewing... https://pharo.fogbugz.com/default.asp?13007 "Cannot integrate a slice with monticello (red square in the UI)"
and in general considering how Monticello uses multiple threads to allow you to type in the filter while its downloading info from the remote repository (which would be good to have more of this sort thing).
Looking back at build 30814 just prior to slice 13007 being integrated, I notice that MCFileRepositoryInspector>>setRepository:workingCopy: [1] forks and sends #refresh, which sends #changed: #packageList.
and MCFileRepositoryInspector>>packageSearchAccept: [2] also forks and sends #changed: #packageList
So there were three threads (including UI [3]) modifying object state, which was compounded by #packageList method doing a lot of work. Issue 13007 worked around this by moving the work of #packageList out to #packageListUpdate, and moved the #changed: sends from [1] & [2] also out into #packageListUpdate (and wrapped in a #defer:).
_QUESTIONS_
Now I am considering doing the same for issue 13182 #versionSearchAccept:, and considering Pharo's goal is to replace all Dependents/#changed: mechanisms with Announcements.
1. So I am wondering if that would be a better direction to address this?
Then I wonder about how Announcements interact with threads. I see that SubscriptionRegistry>>deliver: #protect's the generation of /interestedSubscriptions/, but the actual announcement delivery occurs in the open, in AnnoucemenSubscription>>delvier:, in the sender's thread.
So for an example...
action := [ :it | self inform: 'removing child'.]. announcer on: RemoveChild do: action.
/action/ might be executed from different concurrent threads.
Now you might say that the programmer should know enough about the structure of their threaded code to wrap the action in as critical section, but Announcements are a DECOUPLING mechanism, and maybe a THIRD PARTY library subscribes to an Announcement that later someone else wraps in a fork, and no-one notices possible race conditions created.
2. So, is there something reasonable we can do to make the common/easier/safer single-threaded case the default, and make people work a bit harder to shoot themselves in the foot with multiple threads?
We could introduce Announcer>>onOtherThread:do: with the purpose of making use of inter-thread announcements more explicit in a self documenting way.
Announcements accidentally passed between threads to Announcer>>on:do: would generate an exception advising "use #onOtherThread:do: " with the reasonable expectation that use of #onOtherThread:do: would follow the pattern...
announcer onOtherThread: RemoveChild do: [ :it | myMutex critical: [ self inform: 'removing child' ]. ].
With this arrangement, a third party subscribed to someone else's announcement, that is quietly changed to be wrapped by forked thread, the third party's code now fails EARLY and OBVIOUS, rather than letting subtle race conditions occur at random times.
_PROPOSED IMPLEMENTATION_
"------------------"
Announcer>>subscribe: anAnnouncementClass do: aValuable ^ registry add: ( AnnouncementSubscription new announcer: self; process: Processor activeProcess announcementClass: anAnnouncementClass; valuable: aValuable) "------------------"
AnnouncementSubscription>>deliver: anAnnouncement process ~= Processor activeProcess ifTrue: [ self error: [ 'Cross thread announcements should subscribe using #onOtherThread:do: ' ]. ^ (self handlesAnnouncement: anAnnouncement ) ifTrue: [ [action cull: anAnnouncement cull: announcer] on: UnhandledError fork: [:ex | ex pass ]] "------------------"
Announcer>>threadedSubscribe: anAnnouncementClass do: aValuable ^ registry add: ( ThreadedAnnouncementSubscription new announcer: self; process: Processor activeProcess announcementClass: anAnnouncementClass; valuable: aValuable) "------------------"
where ThreadedAnnouncementSubscription essentially functions the same as the existing AnnoucementSubscription .