There's no need to store preferences in a data structure at all. We 
already have "pragma" preferences (since 4.1), which store the 
preference values independently. Since the 4.1 release it's a "permanent" 
goal to rewrite all preferences to "pragma" preferences.
We should just make it happen.

Levente

P.S.: Reverting that method will solve the concurrency issue.

On Tue, 28 Apr 2015, Eliot Miranda wrote:

> 
> 
> On Tue, Apr 28, 2015 at 12:47 PM, Chris Muller <asqueaker at gmail.com> wrote:
>       Wait, the newer one has a non-local return in it, but
>       Mutex>>#critical: has an ensure: in it anyway, so maybe I don't see
>       the problem..?
> 
> 
> If one hits ctrl-period when the system is in the critical section then the debugger can't open because it interrupts the critical section, preventing the ensure block from running, attempts to access e.g.
> scroll bar preferences when it tries to open, and the system deadlocks.  So preferences either need to be *not* protected by a critical section, or the Debugger needs not to access preferences.
> 
> IMO, we should try and write preferences so that they don't require a lock.  Writing them as a lock-free data structure would be a really good idea. First that critical section is slow and clunky.  Second, I
> presume it is there only for the rare case of a write to preferences, not to protect reads.
> 
> IMO, a simple implementation which copied and replaced the entire preferences dictionary on write would be sufficient.  Sure there's a danger that some client would get a stale value if it read concurrently
> while there was a write, but then so what?  A preference is a preference, not a hard-and-fast value, and code should work accessing a preference no matter its value, so momentarily getting a stale value
> shouldn't matter.  So the implementation could be as simple as
> 
> addPreference: aName categories: categoryList default: aValue balloonHelp: helpString projectLocal: localBoolean changeInformee: informeeSymbol changeSelector: aChangeSelector type: aType
> "Add or replace a preference as indicated.  Reuses the preexisting Preference object for this symbol, if there is one, so that UI artifacts that interact with it will remain valid."
> 
> | aPreference aPrefSymbol |
> aPrefSymbol := aName asSymbol.
> aPreference := DictionaryOfPreferences
> at: aPrefSymbol
> ifAbsent:
> [| newPreference |
> newPreference := aPreference 
> name:aPrefSymbol
> defaultValue:aValue
> helpString:helpString
> localToProject:localBoolean
> categoryList:categoryList
> changeInformee:informeeSymbol
> changeSelector:aChangeSelector
> type: aType.
> AccessLock critical:
> [| newDict |
> newDict := DictionaryOfPreferences copy.
> newDict at: aPrefSymbol put: newPreference].
> self  compileAccessMethodForPreference:aPreference.
> newPreference]
> 
>
>       On Tue, Apr 28, 2015 at 2:43 PM, Chris Muller <asqueaker at gmail.com> wrote:
>       >> The above change restores the old behavior of locking up the image, so it
>       >> should be reverted. An additional comment explaininng why aBlock must not be
>       >> evaluated inside the argument of #accessDictionaryOfPreferencesIn: would be
>       >> helpful.
>       >
>       > Ahh, because aBlock might have a non-local return in it, leaving the
>       > Mutex unsignaled (and critical unenterable), is that right?
>       >
>       > Took me a minute to see that problem.
>       >
>       > Okay, I'll revert that method if no one else does by my next commit..
>       >
>       >> It would be even better to finally get rid of DictionaryOfPreferences.
>       >>
>       >>
>       >> Levente
>       >>
> 
> 
> 
> 
> --
> best,Eliot
> 
>