Comments for rails  

Affected ticket is #638 – sorry. https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/638

Cool, I’m make sure it gets removed before the finished product.

yea let’s just prefix using thread_local_accessor
The reason you have to redefine synchronize is because the default impl of sync is a c function, which calls another c function ‘rb_mutex_lock’ (versus making a ruby method call) to lock/unlock. So you’d actually have to redefine the c functions for lock/unlock (which you can’t do w/o recompiling the interp) to get around redefing sync.
The reason I point this out is that, all that stuff in impl in c for performance. Basically your code kills that perf boost for mutexes in all other libs. So that cost is probably going to be higher than the benefit of being able to check whether you’re inside a mutex.
I do think the idea is cool though. But I also think ensure_thread_safety would only help catch a small subset of race conditions.

The finalizer thing looks nasty, but I don’t have to ever deal with it directly so I don’t really mind.
I think a "active_support" namespace would be the simplest thing to do the job. We can still wrap the namespace with "thread_local_accessor" so you never have to deal with the namespace either.
What do you think of the "lock_with_mutex_counter"? The implementation feels nasty. I really wish this would be some native C method. Maybe we can get it into ruby 1.9 :) (if its a good idea)

https://pastie.caboo.se/183138
Thread.current[x] is faster. It’s pretty much just the finalizer that slows down thread_local_accessor.
The Thread#[](=) impl (in eval.c) is pretty straightforward. Every thread struct has a hash of locals which is then assigned to and read from in the same any other hash is.
In terms of performance it’s a hard call, not sure if it’s worth it. I think prefixing all the thread locals isn’t really that bad a solution. I believe that’s what the DataMapper guys are doing. Also finalizers are quite hated on, regardless if their use is justified. Lots of people have an irrational bias against them.
What do you think?

Yea I’ll check out the impl and do some BMs.

Yeah, I was thinking a thread_accessor method would be cool. Definitely will check it out.
Are there any performance gains to using Thread.current[:var] over @@vars[Thread.current.id]? You should check the actually implementation of the current thread hash.

hey josh… interesting stuff…
We may want to use a prefix in front of the thread local hash keys to prevent collisions with other libraries. For example Thread.current[:asmutex_counter] or something … or my more complicated solution https://coderrr.wordpress.com/2008/04/10/lets-stop-polluting-the-threadcurrent-hash/
What do you think?

Rick, parent links to the previous commit and the merged commit.

Tip: "git merge" takes a ‘-m’ option just like commit, so you can provide a more verbose commit message if needed.

I think it’s fine, unless we can somehow get this commit message to link to the relevant commits that were merged in.

The real commit is here – https://github.com/josh/rails/commit/89237fe07e9c6d35687b3a473f09f4608a3625d8
And yes we are still working out a commit workflow to get the core commits name and author of the patch to show up in the commit log. If you have any good insight please message me.

I think we need better a bit more expressive commit messages than these "Merge branch ‘master’ of git://github.com/...". Isn’t that possible when pushing ?

Got it, that makes sense. So it’s more for your understanding than as a final solution. Also, I did see you remove it in a later commit.

Its fine if you wrap it in a mutex. However, shared memory can cause trouble with threads.
However, all this stuff is really to help me track down the nasty shared memory issues in ActionPack. Most likely will be removed before merging.

What are you trying to accomplish here? Why can’t you modify a class’s attributes in another thread?