On Fri, Oct 26, 2012 at 11:38:19PM +0900, Eregon (Benoit Daloze) wrote:
> 
> Issue #7097 has been updated by Eregon (Benoit Daloze).
> 
> 
> tenderlovemaking (Aaron Patterson) wrote:
> > I spoke with ko1-san and Usa-san last night, and we thought that thread_variable_(get|set) would be good (similar to instance_variable_(get|set)).  I've attached an updated patch to make that change.  The documentation includes examples of how the thread local storage and fiber local storage are different.
> > 
> > I added two more methods:
> > 
> >   * Thread#thread_variables # => returns a list of the defined variable keys
> >   * Thread#thread_variable? # => returns true if a key is set, otherwise false
> > 
> > Thread#local_variable_(get|set) methods respect the same security and frozen behavior as Thread#[] and Thread#[]=.
> 
> Sounds great, but I feel the "thread_" prefix is redundant given it is called on a Thread object.
> I'm thinking to two alternatives:
> 
> * Remove the "thread_" prefix (as usually the thread instance is Thread.current which is very explicit, or the variable name should be clear enough).
> 
>     Thread.current.variable_get(:var)
>     thread.variable_get(:var)
>     worker.variable_get(:var)
>     # versus
>     Thread.current.thread_variable_get(:var)
>     thread.thread_variable_get(:var)
>     worker.thread_variable_get(:var)
> 
> Also, I think get/set do not feel very ruby-like, so ...

If it becomes cumbersome, we can add aliases later.  get/set aren't very
ruby-like, but we have other examples (instance_variable_(get|set)).

> * Use a API resembling fiber locals:
> 
> Thread#locals # => returns an object responding to #[] and #[]= (and maybe #variables and #include?)
> 
>     Thread.current.locals[:var] = some_value
>     Thread.current.locals[:var]
> 
> I guess exposing the whole Hash is exposing internal structures and so is unreasonable. But doing so would be intuitive and avoid having 4 new methods for 4 well-known ([],[]=,keys,include?).

I'd rather not expose another object.  We can't just expose the hash
object because it would have inconsistent behavior with fiber locals:

    irb(main):001:0> t = Thread.new { }.join
    => #<Thread:0x007fe5f11278c8 dead>
    irb(main):002:0> t[:foo] = "bar"
    => "bar"
    irb(main):003:0> t["foo"]
    => "bar"
    irb(main):004:0>

Also, we'd have to figure out what calling `freeze` on that hash means.
Because of these things, we would have to expose a new object that isn't
a Hash.  Exposing a new object means propagating things like
Thread#freeze down to the new object.

If we implement these 4 new method in my patch, our API footprint is
only increased by 4 new methods (vs 5 methods + a new object type).  If
we find later on that exposing an object is a good thing, we can easily
implement these 4 methods in terms of the new object and deprecate the
4 methods.  Going in the other direction, deprecating an object, is not
so easy.

I don't particularly care what the method names are (since we can just
alias them later), but I'm firmly against exposing a new object.

-- 
Aaron Patterson
http://tenderlovemaking.com/